Updated ILMerge to latest version using NuGet #60
Conversation
|
I would prefer to not keep updating binary files inside the repository like this. Would it be possible to instead wrap this tool inside of a NuGet package, and just update this repository to reference the NuGet package? Edit: Please see my comment in issue #53 regarding the NuGet bug. 😄 |
…ited into a repostory.
hubuk
changed the title
There was a problem hiding this comment.
❗ nuget restore should be run prior to entering the MSBuild process as part of the NuGet Automatic Package Restore process. Restoring packages during a build is known to be problematic, and we won't need to do so if we make sure to restore them beforehand.
Summary: This command needs to be removed from this location, and added to the .bat file(s) which launch this build.
There was a problem hiding this comment.
Can you provide some info about what problems it can cause?
There was a problem hiding this comment.
In some cases, the downloaded packages might not be applied to the current build, forcing you to run the build again. Even if that doesn't apply to this project specifically, following the recognized pattern of restore, then build, ensures we will never have problems of this nature.
I believe information is available in the Package Restore NuGet documentation.
There was a problem hiding this comment.
It may be an issue if downloaded package is suppose to modify MSBuild script structure as MSBuild does not allow to mutate already loaded project (source). This is not a case in our scenario.
Functionality you are referring to is called Command-Line Package Restore. Automatic Package Restore is being used by Visual Studio. In addition an approach that is no longer promoted by NuGet team is called MSBuild-Integrated Package Restore. This is the one used to download Enterprise Library package in Code Contracts solution.
Along with moving nuget restore to a bat file it would be required to handle nuget errors. With current approach MSBuild is taking care of that.
There was a problem hiding this comment.
The MSBuild-Integrated package restore will be migrated to automatic package restore as part of a separate commit. At least the VSIX projects will rely on command-line package restore before the build to download packages which integrate into the build process, so we're going to have to run the restore from the .bat file eventually anyway. I prefer to keep it consistent even if running the restore within MSBuild would not cause problems for some subset of the projects.
There was a problem hiding this comment.
If you are going to build VSIX projects using task from within buildMSI10.xml or any other project file you do not need to run nuget from bat file. You can just use taks before . In that way any modification introduced by nuget packages will be processed by MsBuild after (nuget restore) finish.
Maybe we should talk about how the build process should be organized? Currently Editor Extension is not being built with Code Contracts. It would be best to have them build together. If we choose to use command line as a glue for them we should also consider Power Shell.
There was a problem hiding this comment.
All of my command line builds are PowerShell-driven, so you certainly wouldn't hear any complaint from me for that. 😄
There was a problem hiding this comment.
@hubuk I want to see this pull request merged. To get us back on track to do so, can you reiterate the reasons why you don't want to move the call to nuget restore outside of this file? My reasons in favor of moving it are the following:
- Command line restore matches the behavior of automatic package restore in Visual Studio. The restore operation is completed before entering the MSBuild process.
- Command line restore works in all scenarios, and is an easily recognized pattern. IMO, it leaves no confusion regarding the manner and timing in which packages are restored during a command line build. Restoring packages in an MSBuild target requires separate consideration of multiple entry points (the selected build targets) and declared dependencies, and runs a risk that a future configuration might force us back to command line restoration anyway.
Summary: I believe command line restore prior to MSBuild is simpler and more robust in the face of unknown project developments in the future. I have higher confidence that we will be happy with it as a long-term solution for integrating NuGet packages into the build.
There was a problem hiding this comment.
OK, I think I understand our different point of view:
Current CC build script is similar to the one presented in my link. In both cases MsBuild is executed twice:
- First one on
build.projin example andbuildMSI10.xmlin CC. - The second one on
BingSearcher.slnin example andCodeContracts.slnin CC.
This first msbuild run acts like a bat file from your point of view. In both cases nuget is being called before second msbuild run. This is a solution proposed in the NuGet link I provided. And it is stated that this solution differs from a legacy one (the one which uses Nuget.targets) in which:
"The previous implementation had a chicken-and-egg problem for packages that want to extend the build process because NuGet restored packages while building the project"
The presented solution does not have this kind of problems because MsBuild for which the packages were downloaded has not been run yet.
So behavior described in your point 1. is exactly the same for my approach.
All the arguments from point 2 can also be applied to all the logic contained in buildMSI10.xml.
I admit, current project file is quite complex and hard to maintain. We should rethink how the build process should be defined. Then I can agree to move all the logic from buildMSI10.xml to sh1 script. Bat file may be too clumsy.
Moving only nuget.exe restore call to a bat file complicates things. Current approach centralizes all the build activities keeping them in one file. When you move nuget.exe call to a bat file moving another activity from an xml to bat before nuget may not be so simple (depends on complexity of the activity to be moved).
There was a problem hiding this comment.
Thanks for the detailed explanation. I find it acceptable, and believe we are on the same page regarding longer-term goals to evaluate the complete build strategy (which may or may not need changes).
|
I think this is definitely on the right track. With the |
|
👍 My immediate concerns have been addressed and @hubuk and myself are on the same page now. @SergeyTeplyakov I think this can be the second PR to merge. |
|
Guys, can anyone update the PR to simplify automatic merge? |
|
Merged manually. |
3 participants
This fixes #53 (ILMerge does not generate body of methods with
delegates/lambda compiled by roslyn).