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

Cleanup bootstrap build #2594

Merged
merged 7 commits into from
Oct 6, 2017
Merged

Cleanup bootstrap build #2594

merged 7 commits into from
Oct 6, 2017

Conversation

AndyGerlicher
Copy link
Contributor

@AndyGerlicher AndyGerlicher commented Oct 5, 2017 •
edited
Loading

Needed for #2595

  • Download NuGet.Build.Tasks from package (myget feed).
  • Cleanup bootstrap. Avoid copying BuildTools blindly into the output folder.
  • Correct some path properties. A lot of things just worked because we copied so much.

@AndyGerlicher
More cleanup
1c02679 
* Download NuGet.Build.Tasks from package (from myget feed)
* On non-windows don't use net45 tasks path.
@AndyGerlicher
Use $(BuildToolsTaskDir)
0376be4
@AndyGerlicher
Use MSBuildRuntimeType over OsEnvironment
6924f53 
Although this isn't always defined, it always will be defined when
running in `Full`.
@AndyGerlicher
Copy link
Contributor Author

@dotnet-bot test Ubuntu16.04 Build for CoreCLR please

@AndyGerlicher AndyGerlicher changed the title [WIP] Cleanup bootstrap Cleanup bootstrap build Oct 5, 2017
AndyGerlicher
AndyGerlicher commented Oct 5, 2017
View reviewed changes
targets/BootStrapMSBuild.proj Outdated
<NuGetCommonExtensions Include="$(ProjectDir)packages\nuget.protocol\$(NuGetVersion)\lib\net45\**\*.*" />
<NuGetCommonExtensions Include="$(ProjectDir)packages\nuget.versioning\$(NuGetVersion)\lib\net45\**\*.*" />
<NuGetCommonExtensions Include="$(ProjectDir)packages\nuspec.referencegenerator\$(NuGetVersion)\lib\net45\**\*.*" />
<NuGetCommonExtensions Include="$(ProjectDir)packages\Newtonsoft.Json\9.0.1\lib\net45\**\*.*" />

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 really bad, but I hate to do anything fancy here in hopes that SDK migration will happen and this will need to change completely anyway.

jeffkl
jeffkl approved these changes Oct 5, 2017
View reviewed changes
targets/BootStrapMSBuild.proj Outdated
@@ -46,6 +46,23 @@
<InstalledMicrosoftExtensions Include="$(MSBuildExtensionsPath)\Microsoft\**\*.*" />

<InstalledNuGetFiles Include="$(MSBuildExtensionsPath)\Microsoft\NuGet\*" />
<!--<NuGetCommonExtensions Include="$(VsInstallRoot)\Common7\IDE\CommonExtensions\Microsoft\NuGet\**\*.*" />-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

cdmihai
cdmihai reviewed Oct 6, 2017
View reviewed changes
NuGet.Config
@@ -11,7 +11,7 @@
<add key="nuget.org v2" value="https://nuget.org/api/v2/" />
<add key="myget.org roslyn nightly" value="https://dotnet.myget.org/F/roslyn/api/v3/index.json" />
<add key="dotnet.myget.org buildtools v3" value="https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json" />
<!-- <add key="myget.org nugetbuild" value="https://www.myget.org/F/nugetbuild/api/v3/index.json" /> -->
<add key="myget.org nugetbuild" value="https://dotnet.myget.org/F/nuget-build/api/v3/index.json" />
Copy link
Contributor

@cdmihai cdmihai Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Remove comment above . Seems unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I un-commented this (and updated the feed URL) :)

cdmihai
cdmihai reviewed Oct 6, 2017
View reviewed changes
targets/BootStrapMSBuild.proj Outdated
@@ -46,6 +46,23 @@
<InstalledMicrosoftExtensions Include="$(MSBuildExtensionsPath)\Microsoft\**\*.*" />

<InstalledNuGetFiles Include="$(MSBuildExtensionsPath)\Microsoft\NuGet\*" />
<!--<NuGetCommonExtensions Include="$(VsInstallRoot)\Common7\IDE\CommonExtensions\Microsoft\NuGet\**\*.*" />-->
<!-- TODO: Need NuGet.Build.Tasks.dll, would be not to not list out all of its dependencies! -->
Copy link
Contributor

@cdmihai cdmihai Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Comment is unclear: would be not to not

cdmihai
cdmihai reviewed Oct 6, 2017
View reviewed changes
targets/BootStrapMSBuild.proj Outdated
@@ -46,6 +46,23 @@
<InstalledMicrosoftExtensions Include="$(MSBuildExtensionsPath)\Microsoft\**\*.*" />

<InstalledNuGetFiles Include="$(MSBuildExtensionsPath)\Microsoft\NuGet\*" />
<!--<NuGetCommonExtensions Include="$(VsInstallRoot)\Common7\IDE\CommonExtensions\Microsoft\NuGet\**\*.*" />-->
<!-- TODO: Need NuGet.Build.Tasks.dll, would be not to not list out all of its dependencies! -->
<NuGetCommonExtensions Include="$(ProjectDir)packages\nuget.build.tasks\$(NuGetVersion)\lib\net45\**\*.*" />
Copy link
Contributor

Choose a reason for hiding this comment

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

What was wrong with the initial way of getting them from under VS?

Potential problem with grabbing them from their packages is if they have a dependency closure that isn't covered by the enumerated packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a new task from NuGet so we need to latest version. Getting it from VS won't work until that dependency has flowed through. Plus this is better anyway not to copy stuff from VS.

cdmihai
cdmihai reviewed Oct 6, 2017
View reviewed changes
dir.props
<BuildToolsTaskDir>$(ToolsDir)net45/</BuildToolsTaskDir>
<PackagingTaskDir>$(ToolsDir)net45/</PackagingTaskDir>
<BuildToolsTaskDir Condition="'$(MSBuildRuntimeType)'=='Full'">$(ToolsDir)net45\</BuildToolsTaskDir>
<BuildToolsTaskDir Condition="'$(MSBuildRuntimeType)'!='Full'">$(ToolsDir)</BuildToolsTaskDir>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work in our .net core builds because the bootstrap msbuild coming from build tools is so old that MSBuildRuntimeType is empty and therefore different from "Full"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't download MSBuild when on Full (machine install on the image). We only download it when on Core. So == Core would not work, but != Full should.

@AndyGerlicher AndyGerlicher merged commit f3db846 into dotnet:master Oct 6, 2017
@AndyGerlicher AndyGerlicher mentioned this pull request Oct 8, 2017
Merged
radical pushed a commit to mono/msbuild that referenced this pull request Oct 25, 2017
@AndyGerlicher @radical
Cleanup bootstrap build (dotnet#2594)
64fb672 
* Download NuGet.Build.Tasks from package (from myget feed).
* Avoid copying BuildTools blindly into the output folder.
* On non-windows don't use net45 tasks path.
@AndyGerlicher AndyGerlicher deleted the bootstrap-cleanup branch June 26, 2018 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdmihai cdmihai cdmihai left review comments

@jeffkl jeffkl jeffkl 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.
4 participants
@AndyGerlicher @cdmihai @jeffkl @dnfclas