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

onBoard package validation #35846

Merged
merged 3 commits into from
Sep 20, 2021
Merged

onBoard package validation #35846

merged 3 commits into from
Sep 20, 2021

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Aug 27, 2021

Fixes #33981

How/When can we get this version updated automatically? I believe it comes from dotnet/sdk right now - we could add a back-edge subscription that only fires once a week or so?

yes adding a toolset dependency to dotnet/sdk would be the best way to get the most recent changes. This tool is now part of the sdk as well so you can get that by using the daily sdk bits as well

What else should we do to validate that this is doing what we expect? I see the below in a binlog of one of our shipping projects

sadly there is no other way that to check the logs to verify. We talked about adding something but it the eventual decision there was no

The file contention issues faced earlier in #34122 got resolved by dotnet/sdk#20136

we should also port this change to release/6.0 branch

We will need an updated sdk for this one

@Anipik Anipik requested review from dougbu, Pilchie and a team as code owners August 27, 2021 18:40
@Anipik Anipik requested review from wtgodbe, ericstj and joperezr August 27, 2021 18:47
@dougbu
Copy link
Member

dougbu commented Aug 27, 2021

We will need an updated sdk for this one

We normally update the .NET SDK on Mondays but it's Just Fine:tm: to do it in this PR. Feel free to grab the latest.

@joperezr
Copy link
Member

We will need an updated sdk for this one

That is not necessarily true right? In theory they should be fine with a previous SDK as long as they add a PackageReference to Compatibility package as well as manually add the PackageDownload if they desire to do baseline testing right?

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 30, 2021
@wtgodbe
Copy link
Member

wtgodbe commented Sep 1, 2021

I'm not sure what's up with the source build errors:

/__w/1/s/.dotnet/sdk/6.0.100-rc.1.21426.23/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(246,5): error NETSDK1004: Assets file '/__w/1/s/artifacts/source-build/self/src/artifacts/obj/Microsoft.Extensions.Caching.SqlServer/project.assets.json' not found. Run a NuGet package restore to generate this file. [/__w/1/s/artifacts/source-build/self/src/src/Caching/SqlServer/src/Microsoft.Extensions.Caching.SqlServer.csproj]

@dseefeld any ideas?

@dseefeld
Copy link

dseefeld commented Sep 1, 2021

I'm not sure what's up with the source build errors:

/__w/1/s/.dotnet/sdk/6.0.100-rc.1.21426.23/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(246,5): error NETSDK1004: Assets file '/__w/1/s/artifacts/source-build/self/src/artifacts/obj/Microsoft.Extensions.Caching.SqlServer/project.assets.json' not found. Run a NuGet package restore to generate this file. [/__w/1/s/artifacts/source-build/self/src/src/Caching/SqlServer/src/Microsoft.Extensions.Caching.SqlServer.csproj]

@dseefeld any ideas?

It looks to me like this is hitting the same issue described in dotnet/arcade#7795. Looking at the binlog, Microsoft.Extensions.Caching.SqlServer.csproj is marked ExcludeFromSourceBuild, but some targets are still being executed for that project.

@ericstj
Copy link
Member

ericstj commented Sep 1, 2021

Likely has to do with how the validation targets are sequenced + how arcade disables the project entrypoints.

Arcade disables entrypoints by overriding them:
https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/src/Microsoft.DotNet.Arcade.Sdk/sdk/Sdk.targets#L14-L15
https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/src/Microsoft.DotNet.Arcade.Sdk/tools/Empty.targets#L16-L21

However the targets still exist, so things sequenced before/after will still run. I think we can workaround this by conditioning validation not to run when things are excluded from source-build. Perhaps there is some secondary property we can use to do this? Looks like today these targets look at IsPackable and that happens to be set to true, even in the case that source-build is disabling stuff.

@dseefeld
Copy link

dseefeld commented Sep 1, 2021

I opened a PR in Arcade to skip these targets when ExcludeFromSourceBuild is true. dotnet/arcade#7825 @ericstj Is the workaround that you're proposing a general fix for this or just for this PR? If general, maybe my change in Arcade is not needed?

@ericstj
Copy link
Member

ericstj commented Sep 1, 2021

I'm not sure we need a change in Arcade yet. I'd like someone to observe the state of the project when ExcludeFromSourceBuild to see if there is something that we should use as a condition to determine when to run validation.

It seems to me that there ought to be a crisp contract around project state (EG: a single property) that should indicate "I am building and producing a package". We should establish what that is, and if necessary make the changes to honor it, either in package validation or arcade. Once we have such a property we can use that in any Target that might hang it self off of Build, Pack, or Publish to turn itself off during source build.

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Anipik Anipik merged commit 975769f into dotnet:main Sep 20, 2021
@ghost ghost added this to the 7.0-preview1 milestone Sep 20, 2021
@Anipik
Copy link
Contributor Author

Anipik commented Sep 20, 2021

/backport to release/6.0

@ghost
Copy link

ghost commented Sep 20, 2021

Hi @Anipik. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1254491550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Package Validation SDK
7 participants