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

Convert projects to .NET SDK Format #1710

Merged
merged 113 commits into from Nov 1, 2018
Merged

Convert projects to .NET SDK Format #1710

merged 113 commits into from Nov 1, 2018

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Aug 27, 2018

This is intended to supersede #1703 and add in the changes necessary to get our current CI process going again. It may take in some other PRs over time too.

@jen20 jen20 self-assigned this Aug 27, 2018
@jen20 jen20 requested a review from shaan1337 August 27, 2018 19:16
@jen20
Copy link
Contributor Author

jen20 commented Aug 27, 2018

cc @jageall

Copy link

@enricosada enricosada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some feedback, using default and .net core sdk (instead of mono to compile) make it a lot easier to upgrade later to multitargeting

@@ -222,7 +222,13 @@ function buildEventStore {
patchVersionFiles
patchVersionInfo
rm -rf bin/
xbuild src/EventStore.sln /p:Platform="Any CPU" /p:DefineConstants="$DEFINES" /p:Configuration="$CONFIGURATION" || err
msbuild \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use .net core sdk to build also net461.
so in the build script you can use dotnet build, dotnet pack, etc. who are easier to use (and same command line experience for users).

That also mean you dont need to use msbuild of mono for compiling, so you can use mono just as runtime.
It make it easier to upgrade (the .net sdk can be installed side by side, easier to upgrade and pin with a global.json)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @enricosada! Thanks for looking over this and providing feedback. We're actually using dotnet msbuild at the moment internally, but the build scripts are due for a rewrite once the rest of the cleanup work is done, so that hasn't been ported into the old ones yet (the change you are commenting on will not be in the eventual version of this pull request).

The only slightly weird thing appears to be that to build with Mono it is necessary to set FrameworkPathOverrides (or something similar, I forget the exact variable and am on a Windows box right now so don't have it in history).

Ultimately we want to fit into the "standard" .NET workflow as much as possible given the native dependencies.

<TargetFramework>net46</TargetFramework>
<Platform>x64</Platform>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
<OutputPath>..\..\bin\tests\</OutputPath>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that make a lot harder to run dotnt test with multitargeting (useful later to test both net461 and netcoreapp)

using dontet test is easier also from command line and build scripts, no need to use custom test runner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly legacy from the current build scripts, and will be changed as per your comment.

<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
<OutputPath>..\..\bin\tests\</OutputPath>
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new mono support portable too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting - we should likely switch to use that.

<Optimize>false</Optimize>
<TargetFramework>net46</TargetFramework>
<Platform>x64</Platform>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
<OutputPath>..\..\bin\intermediate\</OutputPath>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing like that make use of multitargeting a lot harder later.
new sdk use bin/{Configuration}/{tfm} to make it easier to do incremental build

Copy link
Contributor Author

@jen20 jen20 Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - This is left over from the previous build script set and was ported over - it should be fixed reasonably shortly.

@jen20
Copy link
Contributor Author

jen20 commented Sep 4, 2018

Thanks for the feedback @enricosada! As the replies to individual comments hopefully show, you've caught things in an intermediate state right now - we're moving in the direction that you are advocating for and will be there before this is merged.

Right now there are a few other pieces of cleanup to do first in order to remove compile-time platform detection, and then I'm going to fix up these bits.

@enricosada
Copy link

@jen20 sure! 👍 to the work

jageall and others added 11 commits September 18, 2018 09:41
Removes some files not included in the existing projects
 - EventStore.Core.Tests/Index/IndexV3/table_index_hash_collision_when_upgrading_to_64bit.cs
 - EventStore.Projections.Core.Tests/ClientAPI/when_handling_created/with_from_all_any_foreach_projection/when_running_and_events_are_indexed.cs
 - EventStore.Projections.Core.Tests/ClientAPI/when_handling_deleted/with_from_all_any_foreach_projection/when_running_and_events_are_indexed.cs
 - EventStore.TestClient/Commands/RunTestScenarios/ProjGenerateSampleData.cs
 - EventStore.TestClient/Commands/RunTestScenarios/ProjectionsScenario1.cs
This commit uses msbuild in preference to xbuild on *nix systems. We
also call the restore target prior to the compile target.
 - moved Eventstore.Rags under src
 - dependencies are now pulled from myget feed
- Pin version of mono to be installed to 5.16 on OS X
- Pin version of mono to be installed to 5.16 on Centos 7 image

Co-authored-by: Avish Cheetaram <avishcheetaram.ai@gmail.com>
Add links to projection library build instructions
Add instructions to build UI
Add Azure Pipelines status badge
Remove outdated information
shaan1337
shaan1337 previously approved these changes Nov 1, 2018
@shaan1337 shaan1337 merged commit be95ac0 into master Nov 1, 2018
@jen20 jen20 changed the title [WIP] Convert projects to .NET SDK Format Convert projects to .NET SDK Format Nov 2, 2018
@jen20 jen20 deleted the new-project-format branch November 2, 2018 20:43
@shaan1337 shaan1337 mentioned this pull request Nov 12, 2018
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.

None yet

6 participants