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

Use Cake for build and overhaul/cleanup build #1589

Merged
merged 4 commits into from
Jan 21, 2017
Merged

Use Cake for build and overhaul/cleanup build #1589

merged 4 commits into from
Jan 21, 2017

Conversation

dbent
Copy link
Member

@dbent dbent commented Feb 16, 2016

@pjf Cake (C# Make) is a build system that uses C# as its scripting language. This PR allows CKAN and NetKAN to be built using Cake.

On any system to build CKAN:

>./build ckan

On any system to build NetKAN:

>./build netkan

On any system to build both:

>./build

New

  • build.cake this is the actual Cake build script. I wrote it to do pretty much what bin/build was doing. It performs the following operations:
    • Import ILRepack from NuGet
    • Read some arguments from the command line:
      • --target: What task to execute, if not specified defaults to Build.
      • --configuration: The build configuration to use, if not specified defaults to Debug.
      • --solution: The solution file to build, if not specified defaults to CKAN.sln
      • Define four tasks and their dependencies:
        • BuildDotNet, this generates Meta.cs and runs DotNotBuild which executes msbuild on Windows platforms and xbuild on Unix-like platforms.
        • BuildCkan, this builds ckan.exe
        • BuildNetkan, this builds netkan.exe
        • Build, a wrapper tasks that does nothing itself but depends on BuildCkan and BuildNetkan.
      • Run the specified target using RunTarget(string)
      • Define a couple of helper methods, IsStable() and GetGitVersion()
  • build.ps1 and build are shims to run Cake. build.ps1 is written in PowerShell for Windows systems and build is written in POSIX-compliant shell script (not bash) for Unix-like systems. Both scripts are basically identical and perform the same general steps:
    • Get a copy of NuGet if it's not already available locally
    • Install Cake from NuGet (the version of Cake to use is stored in packages.config, a standard NuGet package configuration and is pasred by the shims)
    • Build the arguments that should go to Cake, since Cake does not yet support getting positional arguments and it's nice to just be able to do > ./build <target> parse the arguments to make that possible.
  • Travis now uses Cake to build.
  • Add environment variables for build configuration. Only use the Debug configuration for now until I can properly configure the existing projects.

Differences

  • I'm using ILRepack 2.0.12 (the latest) because the version 1.25.x that was being used had a bug (Merging assemblies with a reference to Autofac gluck/il-repack#58) that was causing an issue on Windows.
  • Instead of copying the projects over I'm leaving them in place for simplicity. Because of this I don't want to edit Meta.cs in place, so instead I have a Meta.cs.in template file which produces a Meta.cs on build. Meta.cs is then removed from source control and added to .gitignore.
  • This builds puts some things in a .build top-level directory. The eventual goal is to get all build artifacts into this directory so that performing a clean build is a simple rm -r .build and only one entry needs to be in our .gitignore
  • Get rid of all extraneous build scripts I could find (Makefile, build.sh, etc.)
  • Remove all NuGet packages from source control. All packages are now stored in the (unversioned) .build/lib/nuget directory and all project references have been fixed to refer to that directory. A nuget.config file has also been added which will install packages in that directory by default. Yay for no more duplicate NuGet packages! (packages/ has also been removed from .gitignore).
  • Create lib/ for CurlSharp since it doesn't seem to have an official NuGet release. I want to eventually get away from CurlSharp anyway and consolidate all network code (working on that in another branch...)
  • Remove all project specific solution files, there is now only one top level solution file (CKAN.sln).
  • All build output is now directed to .build/out and the ILRepack'd executables are now directed to .build/repack. The Travis CI configuration has been updated to reflect these changes.
  • Building in both Debug and Release configurations is now properly supported. Debug is default, to build in Release use ./build --configuration=Release. Travis CI builds in both but only deploys in Release.
  • Move all duplicate assets to top level assets directory
  • Add CKAN icon to NetKAN executable

Motivation

  • Right now building CKAN and NetKAN on Windows is a bit of a pain, Visual Studio can obviously build it but doesn't do the ILRepack steps to produce a consolidated executable. Setting up a POSIX-like environment to get the standard build script to run is also non-trivial. This should make it trivial to do so by executing ./build in PowerShell.
  • CKAN contributors will certainly have to know C#, but may not know Perl or Python to help with the build system. This makes our build system use the same language as our codebase so anyone contributing to the codebase can also improve the build.
  • There's a lot of cruft and rough edges in the way the build is currently done, this is a first step in consolidating everything into One Build To Rule Them All™. Once this in place I can work on getting packages and tools out of source control, create a semantic changelog, and reorganize the projects and their output for clarity.

@dbent dbent mentioned this pull request Feb 16, 2016
@Dazpoet
Copy link
Member

Dazpoet commented Feb 20, 2016

I can only talk about the reasoning in this PR since code isn't really my forté and henceforth I cannot discuss the potential coding issues that might/might not arise down the line.

The reasoning does seem on point to me. Lowering the barrier to entry for contributing to all parts of CKAN should be something of a prime directive for us and would work in the best interest of our users since it means more contributors and thereby more fresh ideas and added features.

As a Windows user I've also long envied how easy it seems to build the expected .exe files in a POSIX environment and would love the same ease in my own preferred OS.

@Olympic1 Olympic1 added Build Issues affecting the build system Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) labels Feb 28, 2016
@dbent dbent force-pushed the feature/cake branch 3 times, most recently from 72025bb to d738cd5 Compare March 14, 2016 02:53
@mathuin
Copy link
Contributor

mathuin commented Apr 12, 2016

If the goal is to easily build CKAN on Windows, I have a PR #1652 which uses Docker to build and run CKAN. Docker has made great strides in usability on Windows and Mac -- https://docs.docker.com/engine/installation/windows/ shows how it's done. The Docker Compose file that I use to actually run CKAN would need tweaking for Windows but all you need to do is use the location of your target KSP installation in place of the Linux default which is in that file!

@politas
Copy link
Member

politas commented Jan 7, 2017

Ok, so this renames the existing "build.sh" used by Gnu make to "build", and adds the build.ps1 script used to kick off cake on Windows, but for starting cake on Llinux/MacOS, there should be a build.sh (or some renamed version).

@Ruedii
Copy link

Ruedii commented Jan 7, 2017

Microsoft now ships a version of GNU Make for Visual Studio as part of an optional package.

We could utilize that to unify the build scripts, and use a tiny shim so that Microsoft's standard make tools hand off the job to GNU Make.

We might want to (mostly just for fun) see if Mono or Visual Studio .Net do a better job with the build process for official binary. However, we should probably chose whichever one builds with fewer bugs.

@dbent dbent force-pushed the feature/cake branch 12 times, most recently from 5ff097a to 58f75df Compare January 7, 2017 17:38
@mathuin
Copy link
Contributor

mathuin commented Jan 15, 2017

I am almost done with a patch for the Dockerfile. @dbent would you prefer I give you the new lines here or would you rather a PR against your fork?

EDIT: there are essentially two changes -- the new RUN command to build CKAN and the script change to support the new location of the binary.

@mathuin
Copy link
Contributor

mathuin commented Jan 15, 2017

Spoke too soon. :-(

Unhandled Exception:
System.IO.FileNotFoundException: Could not load file or assembly 'CKAN, Version=1.22.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies.
File name: 'CKAN, Version=1.22.0.0, Culture=neutral, PublicKeyToken=null'
[ERROR] FATAL UNHANDLED EXCEPTION: System.IO.FileNotFoundException: Could not load file or assembly 'CKAN, Version=1.22.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies.
File name: 'CKAN, Version=1.22.0.0, Culture=neutral, PublicKeyToken=null'

Any idea what's wrong?

@politas
Copy link
Member

politas commented Jan 15, 2017

System.IO.FileNotFoundException: Could not load file or assembly 'CKAN, Version=1.22.0.0,
Any idea what's wrong?

Why is it trying to build 1.22.0? This branch is at 1.22.1 (or 1.22.2 Pre)

@dbent
Copy link
Member Author

dbent commented Jan 15, 2017

15 failed tests on Windows, 0 failed tests on Linux. Looks like it's largely down to \ versus / in paths. That can be fixed later.

Both forward and back slashes are valid path separators on Windows, anything with a / should work fine on Windows.

I am almost done with a patch for the Dockerfile. @dbent would you prefer I give you the new lines here or would you rather a PR against your fork?

PR.

Any idea what's wrong?

No idea without knowing the exact operation(s) being performed. Is this trying to execute .build/repack/ckan.exe?

Why is it trying to build 1.22.0? This branch is at 1.22.1 (or 1.22.2 Pre)

That number is the AssemblyVersion which is used by the runtimes to determine compatibility for dependency resolution. If we follow semver versioning rules (which we really should) then patch releases (ones that only change the 3rd version number) should not change the public API of an assembly. Therefore all 1.22.x releases should be programmatically compatible and interchangeable. To get this behavior the AssemblyVersion only stores the major (1st) and minor (2nd) version numbers. The AssemblyFileVersion stores the major, minor, and patch (3rd) version numbers. The AssemblyInformationVersion which is just a freeform string stores the "full" version: major, minor, patch, prerelease, and build metadata (short commit hash).

Or:

  • AssemblyVersion: 1.2.0.0
  • AssemblyFileVersion: 1.2.3.0
  • AssemblyInformationalVersionAttribute: 1.2.3.0-dev+abcdef

The Version class stores a 4th number, build, which is not used in semver so is always zero.

@dbent dbent force-pushed the feature/cake branch 2 times, most recently from ebf3959 to 729f27b Compare January 15, 2017 15:14
@dbent
Copy link
Member Author

dbent commented Jan 15, 2017

New changes:

  • I've changed the build directory from .build to _build
  • The cake script will now spit out a random quote from quotes.txt after running.

@dbent
Copy link
Member Author

dbent commented Jan 15, 2017

Pushed another change which will copy ckan.exe and netkan.exe to directly below _build after repack.

This is only for user convenience. Automated scripts should still get the files from _build/repack/$CONFIGURATION.

@mathuin
Copy link
Contributor

mathuin commented Jan 16, 2017

Figured out that error. The Docker container relies on CmdLine.exe, which does not appear to be built anymore. Has it been replaced with ckan.exe? A cursory test makes me think yes, but I would like to know for sure before I send you a pull request.

@politas
Copy link
Member

politas commented Jan 16, 2017

ckan.exe has always been the actual final executable after the ilrepack.

The new build system introduces some minor changes:
* New commands are required to build the software.
* The binaries are stored in different places.

The previous version of this Dockerfile was using the wrong binary!

As a bonus, the configuration setting has been abstracted out as
a build argument.  To build CKAN in the Debug config, use this command:

$ docker build --build-arg config=Debug -t ckan-debug .

To use this container, a command line like this will be required:

$ docker run --rm -v ${KSPDIR}:/kspdir ckan-debug install MechJeb

The default configuration setting is Release.
@mathuin
Copy link
Contributor

mathuin commented Jan 16, 2017

Ah, TIL. I found the other one and it always worked for me! :-)

Created that pull request #1988 -- let me know if there's anything else I can do.

@dbent
Copy link
Member Author

dbent commented Jan 16, 2017

I've made some minor changes that make some problematic test fixtures more robust. As far as I can tell they should now be 100% reliable through the ReSharper and CLI test runners. This means you should be able to ./build test on Windows to your heart's content (keeping in mind that unit testing is about 50% of the build time).

Created that pull request #1988 -- let me know if there's anything else I can do.

Cool.

- The mono version used is the latest (currently `4.6.2`)
- A GitHub release is generated if the following conditions are met:
- All previous steps were successful
- The commit is tagged
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on this? Is this a tag we put on the GitHub PR when merging, a tag on the git commit, or a tag in CHANGES.md? I can't find anything that controls this in travis.yml (though I admit to not fully understanding the syntax)

It might be good to be able to create prereleases on GitHub as well as full releases. (Though we'd need to add an option in the client Auto-update code to opt-in to pre-releases) - I'm cool with adding this functionality later.

Copy link
Member Author

@dbent dbent Jan 19, 2017

Choose a reason for hiding this comment

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

Is this a tag we put on the GitHub PR when merging, a tag on the git commit

These are the same thing. The actual mechanics of a release haven't changed. The only thing different is that the version number is now taken from CHANGES.md CHANGELOG.md rather than the tag itself. The tag can literally be anything so long as it exists. But for consistency it ought to match the version in CHANGES.md CHANGELOG.md.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be good to be able to create prereleases on GitHub as well as full releases.

Just tag a prerelease.

Copy link
Member

Choose a reason for hiding this comment

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

The current method is to go to the releases page and create a new release there. So we still do that, but now the version number reported by the client will come from CHANGELOG.md. Ok, I get it. At some point I must grokify how that actually happens.

@politas
Copy link
Member

politas commented Jan 16, 2017

#1989 seems relevant to this, too. I'm not sure what we'd need to do to include this for Mac users.

@dbent
Copy link
Member Author

dbent commented Jan 19, 2017

#1989 seems relevant to this, too. I'm not sure what we'd need to do to include this for Mac users.

No idea, last time I used a Mac in earnest was before the turn of the millennium.

@politas
Copy link
Member

politas commented Jan 20, 2017

@dbent, are you going to merge in #1988? I think I'm cool to merge the whole PR once that's in. I'm looking forward to rebasing my #1971 so I can debug the failing test.

@dbent
Copy link
Member Author

dbent commented Jan 20, 2017

@dbent, are you going to merge in #1988?

Done.

@mathuin
Copy link
Contributor

mathuin commented Jan 20, 2017

Docker works as expected. LGTM!

@politas politas merged commit 1f1f729 into master Jan 21, 2017
@politas
Copy link
Member

politas commented Jan 21, 2017

Ooof! That was more of a headache than I expected. Must remember to merge upstream changes first!

@Dazpoet Dazpoet deleted the feature/cake branch January 21, 2017 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Issues affecting the build system Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants