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

Ported ILMergeHelper.fs to FAKE 5 as Fake.DotNet.ILMerge #2195

Merged
merged 11 commits into from
Nov 14, 2018

Conversation

SteveGilham
Copy link
Contributor

Description

Take the old ILMerge task and update it

  • The ILMerge code is bound to the .net framework, so won't even work under mono on Windows, so the tests are restricted as for FxCop
  • just as with FxCop, I've been using this code in my own build process via #load, so has some operational test validation as well

TODO

I believe these all to be done

  • [] New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)
  • [] unit or integration test exists (or short reasoning why it doesn't make sense)
  • [] boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)
  • [] (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.
  • [] (if new module) the module is in the correct namespace
  • [] (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)
  • [] Fake 5 API guideline is honored

@SteveGilham SteveGilham changed the title Ported ILMergeHelper.fs to FAKE 5 as Fake.Tools.ILMerge Ported ILMergeHelper.fs to FAKE 5 as Fake.DotNet.ILMerge Nov 11, 2018
matthid
matthid previously approved these changes Nov 11, 2018
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Honestly, if every PR would look like this maintaining would be much less work ;)
Thanks!

I added just one minor discussion point about the behavior on mono, we can leave it as-is but I feel like users might not realize why stuff is red.

use __ = Trace.traceTask "ILMerge" primaryAssembly

// The type initializer for 'System.Compiler.CoreSystemTypes' throws on Mono.
// So let task be a no-op marked as failed on non-Windows platforms
Copy link
Member

Choose a reason for hiding this comment

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

Or we throw an exception in order to notify the user
`raise <| NotSupportedException("ILMerge is currently not supported on mono")

…indows platforms, exactly as in the case of FxCop
@matthid
Copy link
Member

matthid commented Nov 14, 2018

I guess I'd prefer throwing exception (as you can disable "Targets" via conditional targets already), but I can add that myself if I don't forget.
Thanks!

@matthid matthid merged commit a009f1b into fsprojects:release/next Nov 14, 2018
@SteveGilham SteveGilham deleted the develop/ilmerge branch November 15, 2018 17:27
@matthid matthid mentioned this pull request Dec 3, 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.

2 participants