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.

Sign up for GitHub

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

Jump to bottom

Update MSBuild packaging #705

Merged
merged 17 commits into from
Dec 14, 2016
Merged

Update MSBuild packaging #705

merged 17 commits into from
Dec 14, 2016

Conversation

DustinCampbell
Copy link
Contributor

@DustinCampbell DustinCampbell commented Dec 13, 2016 •
edited
Loading

This is a fairly significant change to rationalize how MSBuild is packaged with OmniSharp.

  • Remove Microsoft.Build.Runtime from OmniSharp's project.json (since it won't work for Mono).
  • Update build.cake to install Microsoft.Build.Runtime into packages folder
  • Update build.cake to copy msbuild to '.msbuild-net46' and '.msbuild-netcoreapp1.0' folders (these will be used for debug and test)
  • Update build.cake to copy the appropriate msbuild to the artifacts folders during publish. These are copied to an 'msbuild' subdirectory rather than directly in the OmniSharp folder.
  • Add various .NET SDK packages which are used by newer MSBuild builds, which allow the "Sdk" to be specified in the .csproj as an attribute on the Project node.
  • Copy Microsoft.CSharp.Core.targets to the 'msbuild' folder from the Microsoft.Net.Compilers package rather than including the file directly in OmniSharp.
  • Update MSBuildProjectSystem to properly set MSBUILD_EXE_PATH and MSBuildSDKsPath environment variables so that MSBuild can locate itself.
  • Update ProjectFileInfo to set the MSBuildExtensionsPath to the 'msbuild' subdirectory.
  • Add net46 MSBuild for Mono
  • Various changes to build.cake to package the MSBuild for Mono on net46 builds for OSX/Linux
  • Delete a handful of binaries that are incorrectly included in the publish folder for net46 builds on OSX/Linux
  • Update MSBuild tests to not deploy Microsoft.Build.Runtime and use the '.msbuild-net46' and '.msbuild-netcoreapp1.0' folders instead.
  • Ensure that MSBuild environment variables are set properly at debug time.

david-driscoll
david-driscoll approved these changes Dec 13, 2016
View reviewed changes
Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Do we have to do anything to load the runtime or is that part of the additional changes you alluded too?

Otherwise looks good if you want to merge go ahead.

scripts/nuget.cake
/// <summary>
/// Downloads and unzips a NuGet package directly without any dependencies.
/// </summary>
void DownloadNuGetPackage(string packageID, string version, string outputDirectory, string feedUrl)
Copy link
Member

Choose a reason for hiding this comment

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

I think the latest cake build handles these cases, but I'll circle back and look at that this weekend (god I hope)

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 actually dead code. I started out downloading directly and eventually just decided to install a packages.config. I'll delete that in a future update.

@DustinCampbell
Copy link
Contributor Author

Dropping netcoreapp1.0 and moving to the embedded Mono will need to be a separate thing.

@DustinCampbell
Copy link
Contributor Author

DustinCampbell commented Dec 13, 2016 •
edited
Loading

Note that the artifacts have increased in size now that we have to include several .NET SDKs. 😞

@david-driscoll
Copy link
Member

15 and 30 megs isn't too bad

@DustinCampbell
Copy link
Contributor Author

It's a smaller increase on net46, which is good since that's what I want to focus on.

@DustinCampbell DustinCampbell force-pushed the msbuild-package-acquisition branch 3 times, most recently from c871eee to 023e2c1 Compare December 13, 2016 23:57
david-driscoll
david-driscoll approved these changes Dec 14, 2016
View reviewed changes
Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

👍

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs
{
var msbuildFolder = Path.Combine(AppContext.BaseDirectory, "msbuild");

if (!Directory.Exists(msbuildFolder))
Copy link
Member

Choose a reason for hiding this comment

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

This is strict to support the "I'm developing OmniSharp" scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, FindMSBuildFolder(...) supports debug-time and unit tests.

@DustinCampbell
Copy link
Contributor Author

Thanks for the review!

@DustinCampbell DustinCampbell merged commit 399f970 into OmniSharp:new-msbuild Dec 14, 2016
@DustinCampbell DustinCampbell mentioned this pull request Dec 14, 2016
Closed
@DustinCampbell DustinCampbell deleted the msbuild-package-acquisition branch January 19, 2017 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-driscoll david-driscoll david-driscoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DustinCampbell @david-driscoll