-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix incorrect merge suppressing my change to publish CG2 framework #49666
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
Conversation
As David discovered, I apparently made a merge bug in rebasing my CG2 framework switch-over change prior to checking in that actually suppressed CG2 publishing due to losing the property PublishReadyToRunUseCrossgen2. This change fixes that deficiency. Thanks Tomas
davidwrighton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct calc of this needs to use the computed sdk path, not assume its in .dotnet. This will break developers which install a global build of the sdk which has a sufficiently matching dotnet version. (Most of the library developers do this.)
| <PropertyGroup> | ||
| <OriginalDotnetRootValue>$(DOTNET_ROOT)</OriginalDotnetRootValue> | ||
| <DOTNET_ROOT>$(RepoRoot)</DOTNET_ROOT> | ||
| <DOTNET_ROOT>$([System.IO.Path]::Combine('$(RepoRoot)', '.dotnet'))</DOTNET_ROOT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't reliable. The dotnet.cmd tool will not always point at the .dotnet directory. Instead you need to read the directory from artifacts/toolset/sdk.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4th commit. I have also added a temporary instrumentation showing the DOTNET_ROOT location as the installer builds for some reason still fail to find the correct SDK even though I believe lab machines use the default .dotnet location.
|
@davidwrighton - I believe I have addressed your PR feedback and in fact, thanks to Jeremy's advice it's ultimately super simple. I have also analyzed the binlogs on Windows (there are no binlogs produced for Linux / OSX runs, I'll create a tracking issue for that) and I confirmed that |
As David discovered, I apparently made a merge bug in rebasing my
CG2 framework switch-over change prior to checking in that actually
suppressed CG2 publishing due to losing the property
PublishReadyToRunUseCrossgen2. This change fixes that deficiency.
Thanks
Tomas
/cc @dotnet/crossgen-contrib