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.

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

Resolve source-built prebuilts #69900

Merged
merged 12 commits into from
Sep 13, 2023
Merged

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Sep 12, 2023

Contributes to #69847

System.CommandLine is showing up as a source-build prebuilt after the introduction of the BuildHost project:

<PackageReference Include="System.CommandLine" Version="$(SystemCommandLineVersion)" />

This is because the build is loading the version specified by SystemCommandLineVersion and not using the current version of System.CommandLine produced by source-build. This is fixed by ensuring that System.CommandLine is defined in Version.Details. This allows source-build to override that version with the current version.

Other packages have also been added to the prebuilt baseline as they are only prebuilts in the repo's CI build.

Allows for source-build to override the version
@mthalman mthalman requested a review from a team as a code owner September 12, 2023 13:25
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 12, 2023
@mthalman mthalman requested a review from a team as a code owner September 12, 2023 14:08
Co-authored-by: Michael Simons <[email protected]>
@mthalman mthalman changed the title Add System.CommandLine to Version.Details Resolve source-built prebuilts Sep 12, 2023

<!-- The repo has a dependency on this package that shows up as a prebuilt in the repo's CI but in full
source-build this will be overridden. -->
<UsagePattern IdentityGlob="System.CommandLine/2.0.0*" />
Copy link
Member

Choose a reason for hiding this comment

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

Per conversation with @mmitche - the System.CommandLine and Microsoft.DiaSymReader dependencies in Version.Details.xml should be adorned with the sourcebuild attribute. The value in this is two fold. First, it will remove the need to baseline these because the intermediate will get loaded (if this version exists) and eliminate the prebuilt. Second, as a result of the previous, it will build with the SB variant of the dependency which may be slightly different than the MSFT version.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the System.CommandLine 2.0.0-beta4.23307.1 intermediate doesn't exist so we will still need this exclusion.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Missing NuGet feed?

Copy link
Member

Choose a reason for hiding this comment

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

image

dotnet-libraries-transport, and note different version number for the SB intermediate

Copy link
Member Author

Choose a reason for hiding this comment

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

So how is the intermediate version supposed to be referenced in the source?

Copy link
Member

Choose a reason for hiding this comment

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

Add a Version.Details.xml dependency directly on the intermediate package. example - https://github.com/dotnet/sdk/blob/main/eng/Version.Details.xml#L328

@MichaelSimons MichaelSimons self-requested a review September 12, 2023 15:12
@mmitche
Copy link
Member

mmitche commented Sep 12, 2023

@MichaelSimons...dotnet-libraries-transport feed I think

Comment on lines +18 to +20
<!-- Roslyn's source-build CI builds both NetPrevious and NetCurrent. This 7.0 ref pack shows up as
prebuilt only for the repo CI build but not full source-build. -->
<UsagePattern IdentityGlob="Microsoft.AspNetCore.App.Ref/7.0*" />
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, where does roslyn use the aspnetcore targeting pack?

Copy link
Member

Choose a reason for hiding this comment

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

It stems from this -

<SourceBuildTargetFrameworks>$(NetCurrent);$(NetPrevious)</SourceBuildTargetFrameworks>

Copy link
Member

Choose a reason for hiding this comment

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

Looks like maybe some stuff for Razor? src/Tools/ExternalAccess/AspNetCore/Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.csproj

@MichaelSimons
Copy link
Member

Per my inspection, prebuilt report looks good for the prebuilts intended to be fixed by this PR.

@@ -11,6 +11,15 @@
<Sha>01850f2b7bff23e118d1faecb941d770c8e626f2</Sha>
<SourceBuild RepoName="source-build-reference-packages" ManagedOnly="true" />
</Dependency>
<Dependency Name="System.CommandLine" Version="2.0.0-beta4.23307.1">
Copy link
Member

Choose a reason for hiding this comment

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

Does we need to ensure this version always matches the version in Versions.props - https://github.com/dotnet/roslyn/blob/main/eng/Versions.props#L234 ?

Copy link
Member

Choose a reason for hiding this comment

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

dependency flow takes care of that. Those versions aren't updated manually... usually.

@mthalman
Copy link
Member Author

@dotnet/roslyn-infrastructure - This is hitting seemingly known test failures that I believe are unrelated to these changes. PTAL and merge if good. This helps to unblock dependency flow: dotnet/installer#17319

@@ -12,6 +12,7 @@
<add key="dotnet8-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json" />
<add key="dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" />
<add key="dotnet-libraries" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json" />
<add key="dotnet-libraries-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries-transport/nuget/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have so many NuGet feeds?

Copy link
Member

Choose a reason for hiding this comment

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

My guess is that these aren't all needed. I would bet that dotnet6* and dotnet7* can be eliminated. Maybe nuget-build too.

Copy link
Member

Choose a reason for hiding this comment

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

@mthalman mthalman merged commit a1fa652 into dotnet:main Sep 13, 2023
27 checks passed
@ghost ghost added this to the Next milestone Sep 13, 2023
@mthalman mthalman deleted the commandline-prebuilt branch September 13, 2023 17:34
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants