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

Enable source-build pre-built detection #5190

Merged
merged 6 commits into from
May 31, 2023

Conversation

NikolaMilosavljevic
Copy link
Contributor

Bug

Fixes: NuGet/Home#12598

Regression? No

Description

Enables prebuilt detection, that causes build failure for any new prebuilts not in baseline.

Since it's not possible to flow dependencies, i.e. SBRP intermediate package, all prebuilts have to be listed in prebuilt baseline: https://github.com/NuGet/NuGet.Client/blob/dev/eng/SourceBuildPrebuiltBaseline.xml

All .NET packages are excluded with * version, to avoid unnecessary changes in the future - i.e.:
<UsagePattern IdentityGlob="System.Buffers/*" />

All instances of other packages are excluded with specific version, to ensure that version update triggers proper prebuilt update process, i.e.:
<UsagePattern IdentityGlob="Newtonsoft.Json/13.0.1" />

@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner May 25, 2023 16:32
@ghost ghost added the Community PRs created by someone not in the NuGet team label May 25, 2023
@NikolaMilosavljevic
Copy link
Contributor Author

If you get an error in source-build leg pointing at new prebuilt package being detected,
resolve the prebuilt issue before merging your PR.
If that is not possible, do the following:
- contact source-build team
Copy link
Contributor

Choose a reason for hiding this comment

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

I would include the contact info - use the dotnet/source-build-internal github team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in eaaaebc

resolve the prebuilt issue before merging your PR.
If that is not possible, do the following:
- contact source-build team
- add required UsagePattern entry as provided by source-build team
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think we should exclude anything besides resolving the prebuilt and contacting the source-build team. I fear that suggesting creating UsagePatterns is going to lead to the problem we are trying to prevent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - we want source-build team to provide guidance on a case-by-case basis. Will update - thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in eaaaebc

@@ -28,7 +28,7 @@
<Message Text="Finished restoring Arcade" Importance="High" />

<MSbuild Projects="$(ArcadeDir)/SourceBuild/AfterSourceBuild.proj"
Properties="_NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild;ArcadeBuildFromSource=true;CurrentRepoSourceBuildArtifactsPackagesDir=$(ProjectRoot)artifacts/nupkgs/"
Properties="_NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild;ArcadeBuildFromSource=true;CurrentRepoSourceBuildArtifactsPackagesDir=$(ProjectRoot)artifacts/nupkgs/;FailOnPrebuiltBaselineError=true"
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 negatively affect the product source-build? When doing a full source-build, we don't want this repo to stop the build if it has prebuilts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right - it will affect full source-build. I think we should be able to make this change specific to repo source-build. Will update - thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in eaaaebc

@@ -83,4 +83,5 @@ fi

ReadGlobalVersion Microsoft.DotNet.Arcade.Sdk
export ARCADE_VERSION=$_ReadGlobalVersion
export NUGET_PACKAGES=$scriptroot/../../artifacts/source-build/self/package-cache/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used/exist in the full repo source-build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is set in full source-build: https://github.com/dotnet/dotnet/blob/8528c489bcbaeff232b906a3c784016f2de74fe2/build.sh#L248

I think I need to condition this for repo build only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in eaaaebc

NikolaMilosavljevic and others added 2 commits May 26, 2023 08:44
@NikolaMilosavljevic NikolaMilosavljevic changed the title Prebuilts Enable source-build pre-built detection May 27, 2023
@NikolaMilosavljevic
Copy link
Contributor Author

NikolaMilosavljevic commented May 30, 2023

@kartheekp-ms @zivkan can this be approved for CI? This change is specific to .NET source-build.

@jeffkl jeffkl self-assigned this May 30, 2023
@jeffkl
Copy link
Contributor

jeffkl commented May 30, 2023

@kartheekp-ms @zivkan can this be approved for CI? This change is specific to .NET source-build.

Done!

@NikolaMilosavljevic
Copy link
Contributor Author

@kartheekp-ms @zivkan can this be approved for CI? This change is specific to .NET source-build.

Done!

There are some test failures - but since those are on Windows and this PR is only for Linux and source-build, they are unrelated. I see the same failures in other recent PRs.

@jeffkl
Copy link
Contributor

jeffkl commented May 30, 2023

There are some test failures - but since those are on Windows and this PR is only for Linux and source-build, they are unrelated. I see the same failures in other recent PRs.

Yes those test failures are currently known and we'll ignore them. I'll just give the team one more day to review and I'll merge tomorrow. Thanks for the contribution, glad this will prevent us from breaking source build going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable source-build pre-built detection
4 participants