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

add pre-built detection #86151

Merged
merged 4 commits into from
May 18, 2023

Conversation

oleksandr-didyk
Copy link
Contributor

@oleksandr-didyk oleksandr-didyk commented May 12, 2023

Resolves #81468

Existing issues:

CC: @MichaelSimons @ViktorHofer

@ghost
Copy link

ghost commented May 12, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: oleksandr-didyk
Assignees: oleksandr-didyk
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer
Copy link
Member

starting from Arcade version 8.0.0-beta.23260.3 DotNet.Build.Tasks.TargetFrameworks.targets tries to load Nuget.Frameworks from SBRP during the build and fails

We have been relying on NuGet APIs underneath the Microsoft.DotNet.Build.Tasks.TargetFramework package for years. What's the reason for this showing up now?

@oleksandr-didyk
Copy link
Contributor Author

oleksandr-didyk commented May 15, 2023

starting from Arcade version 8.0.0-beta.23260.3 DotNet.Build.Tasks.TargetFrameworks.targets tries to load Nuget.Frameworks from SBRP during the build and fails

We have been relying on NuGet APIs underneath the Microsoft.DotNet.Build.Tasks.TargetFramework package for years. What's the reason for this showing up now?

I added SRBP for Nuget as part of this work, which was fine at first, but after my change in Arcade the build started failing as it was loading the ref pack in

@ViktorHofer
Copy link
Member

source-build logic has a check for test projects & the check complains about src/libraries/pretest.proj. I can find a way to silence the check for this particular case. That, or maybe the proj should not have test in its name in the first place?

Yes, please silence the check. I'm not exactly happy about that validation tbh. We have other shipping projects with "Tests" in its name which currently are excluded from source build as they rely on external prebuilds, i.e. Microsoft.Extensions.DependencyInjection.Specifications.Tests. While those currently don't cause a problem, we might have other such cases in the future which we want to include during source build.

@oleksandr-didyk
Copy link
Contributor Author

oleksandr-didyk commented May 16, 2023

DotNet.Build.Tasks issue - starting from Arcade version 8.0.0-beta.23260.3 DotNet.Build.Tasks.TargetFrameworks.targets tries to load Nuget.Frameworks from SBRP during the build and fails (source); the version where this manifests has only one change, which is bump of DependencyModel to latest, i.e. I probably missed something during testing. Will investigate.

I think I figured out what happened, but will talk it out with Michael to be sure -> DotNet.Build.Tasks.TargetFrameworks package comes with a version of Nuget already inside of it, in the tools directory. This version is the one that gets loaded in.

In this case, it seems that during Arcade build the SBRP version of Nuget got included in the tools directory and crashes the build. I verified this building the latest Arcade version without SBRP and then using that version in runtime

<UsagePattern IdentityGlob="Microsoft.NETCore.App.Runtime.linux-x64/*8.*" />
<UsagePattern IdentityGlob="*Microsoft.DotNet.ILCompiler/*8.*" />

<UsagePattern IdentityGlob="Microsoft.CodeAnalysis.Analyzers/*" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmitche I followed your reasoning for baselining Analyzers in roslyn here as well, since it follows the same pattern of product build utilizing latest version while for repo build we cannot use intermediates (for now) or SBRP. Pinging just to verify if you think this train of thought is correct here as well. Thanks!

eng/Versions.props Outdated Show resolved Hide resolved
@oleksandr-didyk oleksandr-didyk marked this pull request as ready for review May 17, 2023 18:28
@oleksandr-didyk oleksandr-didyk merged commit f346aac into dotnet:main May 18, 2023
@oleksandr-didyk oleksandr-didyk deleted the add-pre-build-detection branch May 18, 2023 08:15
@lewing
Copy link
Member

lewing commented May 18, 2023

in the future please make sure to update the shas too when updating VersionDetails.xml

@oleksandr-didyk
Copy link
Contributor Author

in the future please make sure to update the shas too when updating VersionDetails.xml

My bad, normally I update dependencies via the darc client, which handles this for me, but this time I did it manually. Will make sure to not forget this in the future

@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable source-build pre-built detection
4 participants