Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Remove buildtools #26108

Merged
merged 44 commits into from
Sep 17, 2019
Merged

Remove buildtools #26108

merged 44 commits into from
Sep 17, 2019

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Aug 9, 2019

I think this is finally ready to go. @dotnet/coreclr-infra this removes the dependency on buildtools. Please let me know if you think there's any additional testing I need to do.

  • Makes a number of common test projects SDK-style, and replaces the buildtools prerelease restore target with the restore logic built in to the SDK. For some projects, we currently restore 5.0 assets that are used for netcoreapp3.0 - this required a workaround to switch the TFM in a few cases.
  • Replaces the corefx testhost deps file creation logic with that from the SDK's publish logic.
  • Uses a new IL sdk (as the old one used to make many redundant file copies), and uses an .ilproj to restore ilasm/ildasm up-front.
  • Uses the UpdateVersions logic from arcade instead of buildtools.

This opens up the possibility of further simplifying some of our msbuild imports, but I don't want to do so in this PR as it's already a sizeable change.

@sbomer sbomer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 9, 2019
@@ -17,7 +17,6 @@
<!-- Source of truth for dependency tooling: the commit hash of the dotnet/versions master branch as of the last auto-upgrade. -->
<PropertyGroup>
<CoreClrCurrentRef>5d3c9a7c54c1c59b764de0e2dfb6bbb4ce29476c</CoreClrCurrentRef>
Copy link
Member

Choose a reason for hiding this comment

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

This was mostly used to update ILAsmVersion.txt. I see you are removing that file, should we just remove the MicrosoftNETCoreRuntimeCoreCLRPackageVersion update and make a DarcMaestro subscription for the CoreCLR packages? Also, if that happens, should we delete this file and move anything interesting left to Directory.Build.props? Are there scenarios that depend on this as it currently is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventual goal is to get rid of this file - I'll look into it and may work on that in this change if it makes sense.

@sbomer sbomer force-pushed the removeBuildTools branch 2 times, most recently from a0d55f6 to 285f5de Compare August 15, 2019 17:49
@sbomer sbomer force-pushed the removeBuildTools branch 2 times, most recently from 03f4d2c to f069b2d Compare September 5, 2019 05:31
Instead we use the built-in SDK targets to restore packages. The new
logic will not copy pdbs from package dependencies to CORE_ROOT.
This prevents issues with the old SDK trying to copy ilasm to the
Tools directory for each build of an ilproj.
Use netcoreapp3.0 to ensure that package dependencies show up in the
build output (bin/TargetingPack). This fixes a missing reference for
CscBench, which depends on Microsoft.CodeAnalysis.CSharp.

Also clean up some unnecessary properties.
This library needs to be restored for netcoreapp5.0, and built for
netcoreapp3.0 to match the tests.
There were problems when trying to target 5.0 using the current SDK,
which only supports targeting 3.0 out of the box.

This required using a more recent SDK, as p6 restored framework
references differently. p7 is able to restore a framework reference
with the MicrosoftNETCoreApp version that we desire.
@sbomer sbomer requested a review from jashook September 13, 2019 15:51
@sbomer
Copy link
Member Author

sbomer commented Sep 13, 2019

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sbomer
Copy link
Member Author

sbomer commented Sep 16, 2019

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sbomer sbomer changed the title [WIP] Remove buildtools Remove buildtools Sep 16, 2019
@sbomer sbomer removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 16, 2019
Copy link

@jashook jashook left a comment

Choose a reason for hiding this comment

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

lgtm thank you for this change!


<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />

<Import Project="$(NuGetPackageRoot)\microsoft.dotnet.versiontools.tasks\$(MicrosoftDotNetVersionToolsTasksPackageVersion)\build\Microsoft.DotNet.VersionTools.Tasks.targets" />
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is the only thing that we still have from buildtools?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

Looks Great! Thank you!

@@ -59,14 +31,6 @@
<AltJitArch>$(__AltJitArch)</AltJitArch>

Choose a reason for hiding this comment

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

We can git of this in the future.

</AllResolvedRuntimeDependencies>
<RunTimeDependecyCopyLocalFile Include="@(AllResolvedRuntimeDependencies)" Exclude="@(RunTimeDependecyExcludeFiles)"/>
<RunTimeDependecyCopyLocal Include="@(RunTimeDependecyCopyLocalFile -> '%(File)')" />

Choose a reason for hiding this comment

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

We can fix the typo in the future.


<Copy
SourceFiles="@(RunTimeDependecyCopyLocal)"

Choose a reason for hiding this comment

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

Typo

@sbomer sbomer merged commit 0dbe04c into dotnet:master Sep 17, 2019
sbomer added a commit to sbomer/coreclr that referenced this pull request Sep 18, 2019
After dotnet#26108, this was broken
because the Tools.proj was never restored on the agent running the
finalize-publish job. This fixes the script to do a restore, and adds
a missing import.
sbomer added a commit to sbomer/coreclr that referenced this pull request Sep 18, 2019
After dotnet#26108, this was broken
because the Tools.proj was never restored on the agent running the
finalize-publish job. This fixes the script to do a restore, and adds
a missing import.
sbomer added a commit that referenced this pull request Sep 18, 2019
* Fix update versions logic

After #26108, this was broken
because the Tools.proj was never restored on the agent running the
finalize-publish job. This fixes the script to do a restore, and adds
a missing import.

* Remove UpdatePublishedVersions.ps1 and add darc dependency
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants