Skip to content

[release/2.1] Include transitive refs in meta-package .nuspec files #30639

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

Merged
merged 4 commits into from
Mar 12, 2021

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Mar 4, 2021

@Pilchie
Copy link
Member

Pilchie commented Mar 4, 2021

Does this mean we'll publish a new version of the meta-packages for all patches, with their dependencies flattened?

I'm not against that (it makes things much better for people using 2.1 on .NET Framework for instance, but in the past there were some concerns about needing to update templates, and that affecting the offline package cache size in the SDK, etc.

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 4, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Mar 4, 2021

@Pilchie our 2.1 builds always produce the two meta-packages. This will flatten only flatten dependencies from dotnet/CoreFx and dotnet/core-setup once it's ready for review. Basically, it'll be similar to the 3.1 and 5.0 special cases we have for some dotnet/runtime repos e.g. https://github.com/dotnet/aspnetcore/blob/release/5.0/eng/Versions.props#L173-L186

For now, this PR is

  1. Not inclusive enough because it doesn't cover a few dotnet/CoreFx and dotnet/core-setup dependencies that are not yet referenced directly anywhere
  2. Too inclusive because I updated a bunch of third-party dependencies that land in our meta-packages but are highly unlikely to get new versions we worry about. My preference here is to change their handling only if we need to react to security issues in those dependencies.

@dougbu dougbu force-pushed the dougbu/meta.package.nuspec branch from 56b658c to 2769437 Compare March 8, 2021 02:56
@dougbu
Copy link
Contributor Author

dougbu commented Mar 8, 2021

@dougbu dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Mar 8, 2021
@dougbu dougbu changed the title Include transitive refs in meta-package .nuspec files [release/2.1] Include transitive refs in meta-package .nuspec files Mar 8, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Mar 8, 2021

Waiting for #30729 to work. Looks as if only ProdCon builds work internally for release/2.1☹️ (https://dev.azure.com/dnceng/internal/_build/results?buildId=1026812 included a mess of unexpected errors)

@dougbu
Copy link
Contributor Author

dougbu commented Mar 10, 2021

Do not merge pro-actively. Will rebase on #30729 commits to enable internal testing and validate that.

dougbu added 3 commits March 11, 2021 12:19
- correct manual internal builds
    - need to explicitly set `$(BuildNumber)` property in all build steps
    - rename `$(BuildScriptArgs)` to avoid circular reference
- skip tests by default in internal non-PR builds

nits:
- include new `$(BuildNumberArg)` in `$(SharedFxArgs)`
- run all test jobs in internal pull requests
- however test\SharedFx.UnitTests\SharedFx.UnitTests.csproj always runs as part of `BuildSharedFx` target
- #30279
- ensure dependencies are up-to-date even when PatchConfig.props is empty
- change only CoreFx and core-setup dependencies
- also change transitive references that weren't mentioned before
@dougbu dougbu force-pushed the dougbu/meta.package.nuspec branch from 487b359 to d06bde9 Compare March 12, 2021 18:15
@dougbu dougbu marked this pull request as ready for review March 12, 2021 18:20
@dougbu
Copy link
Contributor Author

dougbu commented Mar 12, 2021

Confirmed this PR does what it says on the tin by comparing https://dev.azure.com/dnceng/internal/_build/results?buildId=1034483 artifacts with our 2.1.25 release. The Microsoft.AspNetCore.App.nuspec file now contains the transitive references from dotnet/CoreFx and dotnet/core-setup.

In last push, I removed #30729 fixes that matter only between branding and baseline updates.

@dougbu
Copy link
Contributor Author

dougbu commented Mar 12, 2021

Can't add reviewers, likely due to GitHub outage. Please review @dotnet/aspnet-build

@dougbu dougbu requested a review from a team March 12, 2021 18:50
@dougbu
Copy link
Contributor Author

dougbu commented Mar 12, 2021

Working again 😀

@@ -230,5 +230,10 @@
<XunitExtensibilityExecutionPackageVersion>2.3.1</XunitExtensibilityExecutionPackageVersion>
<XunitPackageVersion>2.4.0</XunitPackageVersion>
<XunitRunnerVisualStudioPackageVersion>2.4.0</XunitRunnerVisualStudioPackageVersion>

<!-- Dependencies listed only to include otherwise-transitive references in Microsoft.AspNetCore.App.nuspec. -->
<MicrosoftDotNetPlatformAbstractionsPackageVersion>2.1.0</MicrosoftDotNetPlatformAbstractionsPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Are these all the latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The two below are out-of-date. Should I check Microsoft.Extensions.DependencyModel and all of the System.* packages above❔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it'd be better to have the latest for everything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed though I didn't check everything in this file and instead focused on packages CoreFx, core-setup, extensions, and efcore

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.

Seems reasonable

Comment on lines +135 to +136
<MicrosoftNETCoreApp10PackageVersion>1.0.16</MicrosoftNETCoreApp10PackageVersion>
<MicrosoftNETCoreApp11PackageVersion>1.1.13</MicrosoftNETCoreApp11PackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two version variables are only mentioned in external-dependencies.props but I don't want to mess with deleting them

<MicrosoftNETCoreApp20PackageVersion>2.0.9</MicrosoftNETCoreApp20PackageVersion>
<MicrosoftNETCoreWindowsApiSetsPackageVersion>1.0.1</MicrosoftNETCoreWindowsApiSetsPackageVersion>
<MicrosoftNETTestSdkPackageVersion>15.9.0</MicrosoftNETTestSdkPackageVersion>
<MicrosoftNETTestSdkPackageVersion>15.9.2</MicrosoftNETTestSdkPackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and $(SystemNetHttpWinHttpHandlerPackageVersion) may only matter in tests. They just got caught in my NuGet checkification

@@ -230,5 +230,10 @@
<XunitExtensibilityExecutionPackageVersion>2.3.1</XunitExtensibilityExecutionPackageVersion>
<XunitPackageVersion>2.4.0</XunitPackageVersion>
<XunitRunnerVisualStudioPackageVersion>2.4.0</XunitRunnerVisualStudioPackageVersion>

<!-- Dependencies listed only to include otherwise-transitive references in Microsoft.AspNetCore.App.nuspec. -->
<MicrosoftDotNetPlatformAbstractionsPackageVersion>2.1.0</MicrosoftDotNetPlatformAbstractionsPackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed though I didn't check everything in this file and instead focused on packages CoreFx, core-setup, extensions, and efcore

@dougbu dougbu merged commit bffb3ca into release/2.1 Mar 12, 2021
@dougbu dougbu deleted the dougbu/meta.package.nuspec branch March 12, 2021 22:02
dougbu added a commit that referenced this pull request Mar 12, 2021
- left 3 new transitive dependencies out of #30639
@dougbu dougbu added this to the 2.1.27 milestone Mar 12, 2021
dougbu added a commit that referenced this pull request Mar 12, 2021
- left 3 new transitive dependencies out of #30639
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 tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants