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 to sdk-style projects #285

Merged
merged 17 commits into from
Oct 26, 2017

Conversation

erikbra
Copy link
Member

@erikbra erikbra commented Oct 20, 2017

Microsoft introduced a new form of .csproj files related to the introduction of .NET core. These new, simplified files can also be used for "classic" .NET projects. The .csproj format is much simpler, files are globbed by default (no need to name each file of the project in the .csproj), and it has support for having nuget references directly in the .csproj file as <PackageReference> elements, instead of using packages.config.

This is the csproj file format going forward, and the MS tools will be optimized for this format.

I did have a couple of issues, though. The current build process had a few dependencies to old ways of doing stuff.

  1. nuget restore is not called explicitly before build. This is the new/preferred way of doing it
  2. NAnt tasks are hardwired against an old version of msbuild - we need a new one to be able to understand the new csproj format
  3. I couldn't get the build system to work properly for the output folders. I changed the output folders so that you can just build with msbuild /p:Configuration=Build and everything works, but the UppercuT things I couldn't get to work, I need some help.
  4. I do think that UppercuT (or possible it's the Nant coupling) is extremely tiresome to work with. We have a build process consisting of 4-5 steps, and modifying the templates in XML is really a drag. I created an alternative build_new.ps1, which does everything up to and including ILMerge, and I produce a running rh.exe. But I don't expect you to accept the pull request as is, as the standard build.bat doesn't work properly. I would love if @ferventcoder could take a look at what's wrong.

I would appreciate some feedback from both @BiggerNoise and @ferventcoder on this PR. Is this an OK size for a PR related to #284, or is it too big? The problem is, Nuget package versions are tightly coupled to Nuget version, which is coupled to MSBuild version, which is coupled to the .csproj file format. So is's difficult to change one of them in isolation.

Not used widely anymore elsewhere, and newest version of nuget refs (esp. StructureMap)
is not strong-named anymore
, because the nant task has hardcoded the version for msbuild, and we
need newer.
@erikbra
Copy link
Member Author

erikbra commented Oct 20, 2017

I forgot to mention, I had a problem with strong-naming. The latest versions of Nuget dependencies (i.e. StructureMap) does not come strong named (signed), and will not be releasing any more strong named versions. Strong naming had its place in time, but I don't think it's relevant anymore. Do any of you think it's important to strong-name rh.exe? If we do, there's a nuget package that can be installed in all projects using non-signed nuget packages, which signes them on-the-fly https://github.com/dsplaisted/strongnamer. We can consider using that if you have strong arguments for strong name signing rh.exe.

@BiggerNoise
Copy link
Member

@erikbra - Thank you very much for this. As a rule, I would want P/Rs to be smaller, but, as you said, there's no real good way to break this up. I didn't see anything that looked out of place in the changes.

I had forgotten that we even did strong naming; I'm fine with losing it.

@ferventcoder - I think we need to attach a fair amount of urgency to this P/R. I think the recent burst of activity in RH might motivate others to create P/Rs. Merging P/Rs and this is going to be an epic pain.

Assuming that I can't talk you out of cake based builds, I would propose the following:

  • If you can get the cake going this weekend, add that to this P/R and merge it.
  • Otherwise, let's merge this as is and remove the old uppercut build (use the powershell file to build). We'll add an "Under Construction" blurb to the read me and you can take a couple of weeks to get the real build system in place. That will allow P/Rs to proceed that are unlikely to have massive merge conflicts.

Sound like a plan?

@erikbra
Copy link
Member Author

erikbra commented Oct 21, 2017

Thanks a lot, @BiggerNoise . I do understand the general rule of thumb. But it gets complicated when packages and tools and build systems all depend on each other. I see that I have left out a bit in the powershell based build, I stopped at creating rh.exe. The nugets are never built. But, if you're fine with having powershell based builds for now (an intermediary), I will complete it so that we get an actual deployable out in the other end.

What concerns me, is that we have no build server that can build this, it fails on the one we're using at teamcity.org, because of too old nuget version:

The 'Newtonsoft.Json 10.0.3' package requires NuGet client version '2.12' or above, but the current NuGet version is '2.8.60717.93'.
The 'System.Reflection.Emit.Lightweight 4.3.0' package requires NuGet client version '2.12' or above, but the current NuGet version is '2.8.60717.93'.

Who maintains the build server, @ferventcoder? Is it you? Or is it Jetbrains, in some way?

@BiggerNoise
Copy link
Member

BiggerNoise commented Oct 23, 2017

I have nothing to do with the Team City's stuff, so it is undoubtedly @ferventcoder.

I'd really like to merge this in and update the README to indicate that it is under construction.

@ferventcoder - If you want to add me to the Team Cities account, I can try updating things there. Or if you think you'll have the preferred build up and running shortly that would be great also. I just don't want this P/R to linger.

I'll plan on merging this tomorrow night unless we can develop an alternative plan.

@erikbra
Copy link
Member Author

erikbra commented Oct 23, 2017

I have now fixed the build.ps1 script so that it both adds proper SemVer (using GitVersion.exe - maybe we need to resolve how to install that - should we assume it's installed already via e.g. chocolatey, or should we ship binary in the repo?), and it also generates a .nupkg.

I have chosen a bit different approach to building the nupkg, as msbuild now supports this natively. I added project dependencies to all roundhouse.<databasespecific>.csproj from roundhouse.console.csproj. I don't see any disadvantages of doing this.

Then I added the ILMerge task as part of the roundhouse.console.csproj file. Personally I think this is a lot less complex than using UppercuT, but I don't want to run over you, @ferventcoder, as this is your project. Just, please have a look at how simple (imho of course, but I created it, and might be a bit blind) the build.ps1 file is. It just calls gitversion, msbuild to build and pack, and nunit-console to run tests. No more xml config. Just give it a glance before you go all in on the Cake track.

MSBuild has matured significantly since MS took it in, and I think using the same build script for Visual Studio and cmd line has great advantages. But YMMV.

This won't build on the TeamCity build agent we have, as it requires the newest Visual Studio toolchain (Visual Studio 2017 and Nuget 4.3+).

@BiggerNoise
Copy link
Member

Thanks again for all your work on this. Mondays are really busy for me, so I'll do some roundhousing tomorrow night.

@ferventcoder
Copy link
Member

Otherwise, let's merge this as is and remove the old uppercut build (use the powershell file to build). We'll add an "Under Construction" blurb to the read me and you can take a couple of weeks to get the real build system in place. That will allow P/Rs to proceed that are unlikely to have massive merge conflicts.

Sounds good to me.

@ferventcoder
Copy link
Member

Who maintains the build server, @ferventcoder? Is it you? Or is it Jetbrains, in some way?

Jetbrains. But it's as simple as updating the nuget in the lib folder to a newer version.

@ferventcoder
Copy link
Member

Run with what you got for now. Cake is probably at least a few weeks out.

You are right, the current UC stuff is a pain to work with and it doesn't work with newer infrastructures due to a lack of updated NAnt. And it is unlikely to ever be updated given most folks hate the pain of xml.

Don't forget all the things that go into the packaging parts, that's where much important work is done in physically separating build output from what goes in packages.

</metadata>
<files>
<file src="$mergedExe$" target="bin\" />
<file src="$appConfigFile$" target="bin\" />
Copy link
Member

Choose a reason for hiding this comment

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

Ouch, we need a config file now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe not? Looks like there is mostly bollocks in it. It was in rh 0.8.8 (the latest published one). I can leave it out, if we don't want it.

<file src="$mergedExe$" target="bin\" />
<file src="$appConfigFile$" target="bin\" />
<file src="..\..\nuget\roundhouse\content\ReadMe.RoundhousE.txt" target="content\" />
<file src="..\..\docs\GettingStarted.docx" target="docs\" />
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about leaving this one out?

Copy link
Member Author

Choose a reason for hiding this comment

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

By all means. I saw there was an issue that you should have left out some files to be Chocolatey "compatible" anyway. (#274). I'll take out all but the ones mentioned in that one.

<file src="$appConfigFile$" target="bin\" />
<file src="..\..\nuget\roundhouse\content\ReadMe.RoundhousE.txt" target="content\" />
<file src="..\..\docs\GettingStarted.docx" target="docs\" />
<file src="..\..\docs\legal\*" target="docs\" />
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to move this to next to the binaries, and move the binaries from the bin file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which "this one" do you mean? The legal* ones?

Copy link
Member

Choose a reason for hiding this comment

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

Meaning instead of the docs folder as the location in the nupkg, the license and notice and other legal ones should be beside the binaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an idea, but according to #274, they want the Licence one as LICENCE.txt in the tools folder (together with a VERIFICATION.txt). I see that the NOTICE file is just boiler-plate, so we don't need that, at least.

Copy link
Member

Choose a reason for hiding this comment

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

LICENSE can be brought in as is. It doesn't actually need the .txt, but it's helpful so that it can be read as text on Chocolatey.org package page under the files section.

</metadata>
<files>
<file src="$mergedExe$" target="bin\" />
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of moving this to the tools directory or a subdirectory of that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what chocolatey expects. The nuget of the exe is basically for Choco, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't matter where it is, but what the consistent approach has been for many packages is a subfolder of tools.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the rh.exe itself should be in a subfolder of tools? and then it's the job of chocolateyInstall.ps1 to do what it wants with it (copy to Program Files or AppData or whatever)?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that chocolateyInstall.ps1 is even necessary.

Copy link
Member

Choose a reason for hiding this comment

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

The install.ps1 needs to go away.

Copy link
Member

@ferventcoder ferventcoder Oct 24, 2017

Choose a reason for hiding this comment

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

With just binaries in the package (no scripts), you can run these commands in succession:

  • choco install roundhouse
  • rh

Chocolatey automatically shims executables found in packages and puts them into a folder that is on the PATH ($env:ChocolateyInstall\bin). The shim runs the original exe, so no dependencies are needed to be brought over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. I now know more why i love choco ;)
So, skip the chocolateyInstall.ps1. Where do we put rh.exe? Tools? root?

Copy link
Member

Choose a reason for hiding this comment

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

My vote for consistency is the tools folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we end up with just this in the Nugget (I love calling them nuggets), then?

image

//[assembly: AssemblyKeyFile("..\\..\\RoundhousE.snk")]
Copy link
Member

Choose a reason for hiding this comment

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

Signing is less important nowadays to most folks in the community. The question is whether organizations (the quiet unheard group) still feel the same way. I like the concept of that signer you mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can look into it. It basically signes all unsigned dlls in the project. But we'll need to use it in all the projects, then, bercause the DLL changes (it's basically a different one, as the Assembly qualified name changes).

Copy link
Member

Choose a reason for hiding this comment

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

👍

<CodeAnalysisRuleSet>AllRules.ruleset</CodeAnalysisRuleSet>

<VersionPrefix>0.8.9</VersionPrefix>
<VersionSuffix>alpha</VersionSuffix>
Copy link
Member

Choose a reason for hiding this comment

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

How do these get updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't. I override them on command-line with MSBuild properties when packing (in build.ps1). I generate semantic version using GitVersion.exe (probably same logic as in the nant build files) . Maybe just leave them as a "default" or easy-to-understand-that-is-dummy value in the .csproj?

Do you have any thoughts on external dependencies, such as GitVersion.exe? Should we check in the binaries, or should we expect people to use chocolatey to install themselves?

Copy link
Member

Choose a reason for hiding this comment

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

GitVersion is awesome.

Copy link
Member

Choose a reason for hiding this comment

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

There is a concept of setup, where all the tools are brought to a repository. That setup should give a developer everything they need to do build.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, true. We could make a setup.ps1 which pulls chocolatey from the web, etc, and uses that to install GitVersion. but maybe a bit intrusive. Is it possible to install choco packages without admin? If not, this will definitely not work for build servers, etc. I love having the same script on build servers as local, so that would be nice, to create a setup.ps1, which assures all necessary tools are in place, and then the build.ps1 (or cake, maybe, preferably), could do the build part itself.

nuget it part of the build chain, but the .exe of chocolatey is not in the nuget package (which makes sense).

Copy link
Member

Choose a reason for hiding this comment

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

The good news? Cake does all of this already.

<ILMergeAssemblies Include="$(OutputPath)*.dll" />
</ItemGroup>
<PropertyGroup>
<IgnoreFile>..\..\build.custom\ilmerge.internalize.ignore.txt</IgnoreFile>
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving this somewhere more appropriate over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. We could move all these up to the roundhouse.console project (maybe in a "pack" or "nuget" subfolder).

Copy link
Member

Choose a reason for hiding this comment

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

More of a placeholder comment... just thinking about where they could go, no action to be done now.

</ItemGroup>
<PropertyGroup>
<IgnoreFile>..\..\build.custom\ilmerge.internalize.ignore.txt</IgnoreFile>
<ILMerge>..\..\lib\ILMerge\ILMerge.exe</ILMerge>
Copy link
Member

Choose a reason for hiding this comment

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

Where does this resource come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which resource? ILMerge.exe?

Copy link
Member

Choose a reason for hiding this comment

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

<ILMerge> - typically if you use things from MSBuild that are not included, it can cause failures. It's good to know if this is build into MsBuild or requires a community pack or something.

Copy link
Member Author

@erikbra erikbra Oct 24, 2017

Choose a reason for hiding this comment

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

No, no, is not a task. It is just a property. Maybe I should have called it <ILMergeExe> or something.

Copy link
Member

Choose a reason for hiding this comment

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

I see it now... property group. My bad...

<Output ItemName="BuiltExeOutputGroupOutput" TaskParameter="Include"/>
</CreateItem>

<CreateItem Include="$(ExeProjectOutputDir)\**\*.*" Condition="'$(OutDir)' != '$(OutputPath)'">
<CreateItem Include="$(ExeProjectOutputDir)\**\*" Condition="'$(OutDir)' != '$(OutputPath)'">
Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks for fixing these

@BiggerNoise BiggerNoise reopened this Oct 25, 2017
@BiggerNoise
Copy link
Member

@ferventcoder - I have created an AppVeyor account and sent you the email that I prefer.

@erikbra - I'm having mixed results when I pull this down. The build is fine in visual studio and the command line, but it looks like the console pack and unit test aren't going correctly. This is what I am seeing here:

C:\Program Files\dotnet\sdk\2.0.0\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(141,5): error : Could not find a part of the path 'C:\development\Chuck\roundhouse\code_drop\merge'. [C:\development\Chuck\roundhouse\product\roundhouse.console\roundhouse.console.csproj]

 * Looking for nunit3-console.exe
    - Found at C:\Users\bigge\.nuget\packages\nunit.consolerunner\3.7.0\tools\nunit3-console.exe

 * Running unit tests

dir : Cannot find path 'C:\development\Chuck\roundhouse\build_output\RoundhousE.UnitTests\net461' because it does not exist.
At C:\development\Chuck\roundhouse\build.ps1:42 char:13
+ $tests =  $(dir "$($TESTOUTDIR)\*.tests.dll");
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (C:\development\...nitTests\net461:String) [Get-ChildItem], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetChildItemCommand

Runtime Environment
   OS Version: Microsoft Windows NT 10.0.16299.0
  CLR Version: 4.0.30319.42000

Error: no inputs specified

Unfortunately, I got a late start tonight and my brain is kind of mush. You would expect this to work, right? I installed GitVersion, and added msbuild to my path so I can hit it from powershell; was there something else I was supposed to do?

@BiggerNoise
Copy link
Member

I can see the project in AppVeyor, so gtg there.

@erikbra
Copy link
Member Author

erikbra commented Oct 25, 2017

@BiggerNoise Probably forgot to create some folders if not already existing, in the msbuild script. I'll do a git clean and try again.

@erikbra
Copy link
Member Author

erikbra commented Oct 25, 2017

Try again now, @BiggerNoise . There was some confusion btw Solution configurations (Which are "Build|Any CPU") and project configurations (which are "Release|AnyCPU"). There are no "Build" project configurations. "Build" solution config is set to build the "Release" project configs. In addition, there was the AnyCPU/Any CPU (with blank) confusion, as described here: http://jimmyscorner.com/archives/51/the-differences-between-solution-build-configurations-and-project-build-configurations

This made the folders not align up, as I thought the conditionals on Configuration = "Build|AnyCPU" would hit. They didn't. But now it should work from scratch.

@BiggerNoise
Copy link
Member

@erikbra I don't think that your commits made it to the P/R

@erikbra
Copy link
Member Author

erikbra commented Oct 25, 2017

Blaargh, sorry. I may have forgotten to push. I'll check when I get to that computer in the morning (CET).

@erikbra
Copy link
Member Author

erikbra commented Oct 26, 2017

Yepz, n00b mistake. Commit, move on to something else. Forgot to push. Now it should build from scratch, @BiggerNoise .

@BiggerNoise
Copy link
Member

I'm still having a couple of issues building from scratch, but don't have time to get into them this morning.

I updated the AppVeyor project to run .\build.ps1 (for now).

It's reporting success, but it's lying. Let me know if you have trouble seeing the logs.

@erikbra
Copy link
Member Author

erikbra commented Oct 26, 2017

Yes, it should fail on error output. I don't know how to configure that in AppVeyor, only TeamCity. Seems like some of the folders still don't exist beforehand. I'll see if I can reproduce if nuking my ~/.nuget folder. Didn't think of that one. And code_drop\log\ is missing (some of it), apparently, too.

@erikbra
Copy link
Member Author

erikbra commented Oct 26, 2017

We have too old MSBuild in AppVeyor, @BiggerNoise. We need 15.0. See log from latest build:

C:\projects\roundhouse\product\roundhouse\roundhouse.csproj.metaproj : warning MSB4078: The project file "product\roundhouse\roundhouse.csproj" is not supported by MSBuild and cannot be built.

That's why we don't have a C:\.nuget either. Because the Nuget is probably older as well ( i assume)

@lahma
Copy link
Contributor

lahma commented Oct 26, 2017

@erikbra have tried defining appveyor.yml with vs2017 image? The preview part shouldn't be necessary anymore though.

@ferventcoder
Copy link
Member

@lahma We will get the appveyor.yml file exported to this source once it is all set up. Currently it is defined as follows:

version: 1.0.{build}
image: Visual Studio 2017
build_script:
- ps: .\build.ps1

@lahma
Copy link
Contributor

lahma commented Oct 26, 2017

@ferventcoder Now that I actually read the output from appveyor, it seems that MSBuild should be good to go.

MSBuild auto-detection: using msbuild version '15.4.8.50001' from 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\bin'.

However,
$nunit = $(dir -r $env:HOME\.nuget\packages\nunit* -i nunit3-console.exe | Select-Object -last 1)

Does not seem to resolve to correct dir:
dir : Cannot find path 'C:\.nuget\packages' because it does not exist.

What about:

$(dir -r $env:UserProfile\.nuget\packages\nunit* -i nunit3-console.exe | Select-Object -last 1)

The next errors seems to originate from the fact that databases cannot be contacted, have they been enabled in build configuration?

Appveyor also does some automatic test detection magic so might conflict with build.ps1...

if running in AppVeyor, tests will be automatically discovered and run.
@erikbra
Copy link
Member Author

erikbra commented Oct 26, 2017

I have fixed the Nuget folder problem (look for $env:NUGET_PACKAGES, if not found, use ~/.nuget (works on all platforms)). I have also removed the running of tests manually with nunit-console on AppVeyor (check for $env:AppVeyor), since it seems to pick up the tests itself.

@ferventcoder: You just need to tell it not to run the integration tests, as we don't have any integration tests environments set up yet.

Can be done like this: https://www.appveyor.com/docs/running-tests/#selecting-assemblies-andor-categories-to-test

Probably should use **/*.tests.dll as the filter.

@erikbra
Copy link
Member Author

erikbra commented Oct 26, 2017

We have a lot to go on with regards to test setup, artifacts gathering, etc, but at least we're building on AppVeyor now. I would prefer if we could put the AppVeyor config in source. Easier to maintain. But maybe we should make a separate issue for that, @ferventcoder . We also have some cleanup tasks, to assure all the nuget packages created are OK, etc, etc (I think they're not right now).

I'm at 00:30 CET now, so I'm retiring for the day. I'll let the rest of the world work :)

@BiggerNoise BiggerNoise merged commit 8a51d3a into chucknorris:master Oct 26, 2017
@erikbra erikbra deleted the sdk-style-projects branch October 27, 2017 11:04
@erikbra erikbra mentioned this pull request Oct 27, 2017
@erikbra erikbra added this to the 0.9.0 milestone Nov 13, 2017
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