Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Paket and convert all projects to VS2017 format #324

Merged
merged 34 commits into from
May 3, 2017

Conversation

ceferrari
Copy link
Contributor

@ceferrari ceferrari commented Apr 30, 2017

  • Removed Paket
  • Converted all projects to the new .csproj format (VS2017)
  • Updated all packages to latest versions (except EventStore, which requires Framework v4.6)
  • Fixed all related tests and code
  • Changed target framework to 4.6.2
  • Corrected a few mispellings

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2017

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #324 into develop will decrease coverage by 2.26%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #324      +/-   ##
===========================================
- Coverage     85.3%   83.03%   -2.27%     
===========================================
  Files          242      242              
  Lines         5186     5182       -4     
  Branches       409      409              
===========================================
- Hits          4424     4303     -121     
- Misses         554      677     +123     
+ Partials       208      202       -6
Impacted Files Coverage Δ
...entStores.EventStore/EventStoreEventPersistence.cs 89.01% <100%> (ø) ⬆️
...entFlow/EventStores/Files/FilesEventPersistence.cs 77.77% <100%> (ø) ⬆️
...w/EventStores/InMemory/InMemoryEventPersistence.cs 92.94% <100%> (ø) ⬆️
...tFlow.SQLite/EventStores/SQLiteEventPersistence.cs 89.36% <75%> (ø) ⬆️
...entFlow.MsSql/EventStores/MsSqlEventPersistence.cs 93.81% <75%> (ø) ⬆️
...icsearch/ReadStores/ElasticsearchReadModelStore.cs 80.3% <77.77%> (ø) ⬆️
...Q/Extensions/EventFlowOptionsRabbitMqExtensions.cs 0% <0%> (-100%) ⬇️
...ow.RabbitMQ/Integrations/RabbitMqMessageFactory.cs 0% <0%> (-100%) ⬇️
...EventFlow.RabbitMQ/RabbitMqDomainEventPublisher.cs 0% <0%> (-100%) ⬇️
...RabbitMQ/Integrations/RabbitMqConnectionFactory.cs 0% <0%> (-93.55%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae259d4...c9fa72e. Read the comment docs.

@rasmus
Copy link
Member

rasmus commented May 1, 2017

@ceferrari Impressive piece of work with some nice cleanup and as well 👍 👍 👍

A few initial comments

  • .NET framework 4.5.1 still needs to be supported. A few clients still have some old systems that are compatible with any higher
  • Looking forward to see the resulting NuGet packages

Let me know when its done

@rasmus rasmus mentioned this pull request May 1, 2017
@ceferrari ceferrari changed the title Removed Paket, Updated all packages to latest versions and Changed ta… Remove Paket and update all packages to latest versions May 1, 2017
@ceferrari
Copy link
Contributor Author

ceferrari commented May 1, 2017

Thank you @rasmus! You deserve all the recognition for this great project!

The target framework is already reverted to v4.5.1.

I didn't get what you mean by "see the resulting NuGet packages". Could you clarify please?

Edit: I added a Task named "Restore" to build.cake file. It runs between "Version" and "Build" tasks and will show all restored packages. Is that what you mean by "see the resulting NuGet packages"?

@rasmus
Copy link
Member

rasmus commented May 1, 2017

Thanks @ceferrari for that 😄

Regarding package you should update the appveyor.yml line https://github.com/eventflow/EventFlow/blob/develop/appveyor.yml#L24 with something like **/Release/**/*.nupkg to make sure that the packages are outputted to the AppVeyor artifacts. That way they can be downloaded and inspected if they are correctly formed. They should have the information similar to that of the packages found on http://nuget.org/, especially care regarding the dependency versions. The release notes might be a problem, not sure how to do that (it might be as simple as a msbuild property argument though). There's more details here: https://docs.microsoft.com/en-us/nuget/guides/create-net-standard-packages-vs2017 basically the information from the deleted packages.template files are missing from the csproj.

Let me know if you need some feedback.

@ceferrari ceferrari changed the title Remove Paket and update all packages to latest versions Remove Paket and convert all projects to VS2017 format May 2, 2017
@ceferrari
Copy link
Contributor Author

@rasmus I had to convert all packages to the new .csproj (VS2017) format to make packaging work using the "DotNetCorePack" method from cake. I started with the root project (EventFlow) to make sure the subsequent projects did not have any duplicated references (e.g. If project A references Newtonsoft.Json and Project B references Project A, then Project B does not need to reference Newtonsoft.Json too, it will be transitive)

I guess the packaging will be fine now but GitLink stopped working because it does not support the new .csproj format yet. I don't know what to do about it.

The latest AppVeyor build failed because the MsSql tests failed. They shouldn't tho. If you can re-run they will probably pass.

@rasmus
Copy link
Member

rasmus commented May 3, 2017

@ceferrari I re-run the build and it passed 👍

I created an issue at AppVeyor a few days back to get to the bottom of the SQL problem. They said this is an isolated case, so I'll look into that as soon as I get some time.

It really cool that you did the switch to the new VS2017 project format as well as it should make it really easy to produce .NET standard packages (exchange <TargetFramework>net451</TargetFramework> with <TargetFrameworks>net451;netstandard1.6</TargetFrameworks>). That will require some more code changes so that will have to be another PR.

The NuGet packages reference the project produced NuGet packages version 1.0.0. It might be related to the DLLs missing a version.

Also the NuGet package is missing its details. NuGet.org takes the information from the packages, so if the information is removed, it will be removed from the site as well.

NuGet package

image

DLL details for produced DLLs

image

Reference, NuGet package details downloaded from http://nuget.org

image

Reference, DLL from NuGet downloaded from http://nuget.org

image

@rasmus
Copy link
Member

rasmus commented May 3, 2017

@ceferrari Its a big change, but its looking good 👍

@ceferrari
Copy link
Contributor Author

@rasmus The packages information are fixed now.

Let me know if they match what is expected.

@rasmus
Copy link
Member

rasmus commented May 3, 2017

Hi @ceferrari

Again thanks for putting the work into this. A few minor comments.

  • The NuGet package JetBrains.Annotations should be excluded from the dependencies of the EventFlow package as its only used during development. The final DLL has no reference to it. Documentation is listed here under "Controlling dependency assets": https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files
  • You should be able to remove the Microsoft.Owin.Host.HttpListener from the EventFlow.Owin package
  • Maybe do a search and replace for <PackageProjectUrl>http://geteventflow.net/</PackageProjectUrl> and replace with <PackageProjectUrl>http://docs.geteventflow.net/</PackageProjectUrl> in the csproj files. The documentation site is a better starting point

As for the missing release notes within the NuGet packages details and breaking GitLink, well, I'll have a look at that at some later point. Getting this finalized is important.

@ceferrari
Copy link
Contributor Author

ceferrari commented May 3, 2017

Hi @rasmus

1 - Done

2 - When I do that, the Owin tests fail. The relevant output from both "Ping" and "PublishCommand" tests is:

System.MissingMemberException : The server factory could not be located for the given input: Microsoft.Owin.Host.HttpListener

3 - Done

I will keep an eye on GitLink updates to support the new .csproj files.

@rasmus
Copy link
Member

rasmus commented May 3, 2017

@ceferrari Any last minute changes? If not I'll merge this in

@ceferrari
Copy link
Contributor Author

@rasmus No changes. Go ahead!

@rasmus rasmus merged commit fa315bc into eventflow:develop May 3, 2017
@rasmus
Copy link
Member

rasmus commented May 3, 2017

@ceferrari Thanks, nice PR lost of good stuff 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants