-
Notifications
You must be signed in to change notification settings - Fork 174
Enable source-build prebuilt detection #1821
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
|
cc @dotnet/source-build-internal, @mmitche, @oleksandr-didyk |
mmitche
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.
I think Microsoft.NET.Compilers.Toolset could be unified with the rest of the roslyn dependencies and that would eliminate a huge swath.
Do we need the Microsoft.NET.Compilers.Toolset dependency in this repo at all? It might have been needed in the past but I wonder if we can these days just use the compiler that comes via the toolset. |
Probably not, if the repo has UseToolMicrosoftNetCompilersToolset set to false/unset |
a2987e5 to
513ac1c
Compare
UseToolMicrosoftNetCompilersToolset does not appear to be set. |
The Microsoft.NET.Compilers.Toolset dependency is used to control all roslyn dependencies. Unfortunately the use of MicrosoftNetCompilersToolsetPackageVersion caused the repo to encounter dotnet/source-build#3392 which caused the repo to build with the MicrosoftNetCompilersToolsetPackageVersion coming from the SDK instead of the repo's versions.props. |
|
@ViktorHofer and @mmitche - I has resolved all prebuilts except one that I propose leaving as an exception for now. Please review. TIA |
| <Dependency Name="Microsoft.SourceLink.GitHub" Version="8.0.0-beta.23218.3" CoherentParentDependency="Microsoft.DotNet.Arcade.Sdk"> | ||
| <Uri>https://github.com/dotnet/sourcelink</Uri> | ||
| <Sha>47c52dd2ebf9edfd40abdcff999c13eb461f6ce2</Sha> | ||
| <SourceBuild RepoName="sourcelink" ManagedOnly="true" /> | ||
| </Dependency> |
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.
Do we need to remove these after P4 when sourcelink will be part of the sdk?
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.
I must be living under a rock - this is news to me. If that's the case, then yes all sourcelink dependencies would have to be removed as well as removing the sourcelink repo from the VMR.
Can you point me to an issue for this work?
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.
|
cc @sharwell - Can you review and merge if you approve? TIA. |
ff8a658 to
ef303cf
Compare
|
@sharwell - could you please review and merge if approved? This PR conflicts with every Arcade dependency flow so it is a burden to keep green. TIA |
Fixes #1809
There are some prebuilts remaining as indicated in SourceBuildrebuiltBaseline.xml that still need addressing.