Skip to content

[build] Make package metadata optional #21081

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 6 commits into from
Aug 23, 2024
Merged

[build] Make package metadata optional #21081

merged 6 commits into from
Aug 23, 2024

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Aug 20, 2024

Looking at the package explorer for recent .NET 9 preview 7 builds I
noticed that other packages do not include sha metadata in the version
element. Compare the following by clicking "View Metadata source":

https://nuget.info/packages/Microsoft.iOS.Ref.net9.0_17.5/17.5.9231-net9-p7
https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.ios-arm64/9.0.0-preview.7.24405.7
https://nuget.info/packages/Microsoft.Maui.Sdk/9.0.0-preview.7.24407.4

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
arcade publishing tooling which expects these to be in sync.

While waiting for a fix in arcade, we can remove this metadata to
restore dependency flow. It can be restored in the future by setting
the following:

NUGET_BUILD_METADATA=+sha.$(CURRENT_HASH)

The icon and other package metadata have been updated to better match
other .NET packages.

Looking at the package explorer for recent .NET 9 preview 7 builds I
noticed that other packages do not include sha metadata in the version
element. Compare the following by clicking "View Metadata source":

    https://nuget.info/packages/Microsoft.iOS.Ref.net9.0_17.5/17.5.9231-net9-p7
    https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.ios-arm64/9.0.0-preview.7.24405.7
    https://nuget.info/packages/Microsoft.Maui.Sdk/9.0.0-preview.7.24407.4

I believe the sha metadata was added when we were initially figuring out
a package versioning schema that would produce unique package versions,
however our usage of commit distance in both preview and stable versions
should address this concern.

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
[arcade publishing tooling][0] which expects these to be in sync.

The short hash in the metadata can be be replaced by the `repository`
element which contains the repo url, branch, and full commit hash.

The icon and other package metadata have been updated to better match
other .NET packages.

[0]: https://github.com/dotnet/arcade/blob/b4f4d40741f161e2c0d96c19c51a4013850ef65f/src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@pjcollins pjcollins marked this pull request as ready for review August 20, 2024 23:39
@pjcollins pjcollins requested a review from rolfbjarne as a code owner August 20, 2024 23:39
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

I believe the sha metadata was added when we were initially figuring out
a package versioning schema that would produce unique package versions,
however our usage of commit distance in both preview and stable versions
should address this concern.

I added it to make it very easy to figure out which commit produced any given package (as stated above the semver spec is very clear that metadata should be ignored, so uniqueness didn't factor into this decision).

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
arcade publishing tooling which expects these to be in sync.

I believe this to be a bug in arcade. The semver spec doesn't explicitly say that metadata should be ignored when comparing versions, but it's very clear that metadata should be ignored when comparing version precedence: "Build metadata MUST be ignored when determining version precedence."

I believe arcade should ignore metadata when comparing versions - so is there any way this could be fixed in arcade instead?

@@ -4,7 +4,7 @@

Our NuGet packages are versioned using [Semver 2.0.0][2].

This is the scheme: `OsMajor.OsMinor.InternalRelease[-prereleaseX]+sha.1b2c3d4`.
This is the scheme: `OsMajor.OsMinor.InternalRelease[-prereleaseX].CommitDistance`.

This comment was marked as outdated.

@pjcollins
Copy link
Member Author

I believe the sha metadata was added when we were initially figuring out
a package versioning schema that would produce unique package versions,
however our usage of commit distance in both preview and stable versions
should address this concern.

I added it to make it very easy to figure out which commit produced any given package (as stated above the semver spec is very clear that metadata should be ignored, so uniqueness didn't factor into this decision).

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
arcade publishing tooling which expects these to be in sync.

I believe this to be a bug in arcade. The semver spec doesn't explicitly say that metadata should be ignored when comparing versions, but it's very clear that metadata should be ignored when comparing version precedence: "Build metadata MUST be ignored when determining version precedence."

I believe arcade should ignore metadata when comparing versions - so is there any way this could be fixed in arcade instead?

Thanks for the extra context here. I started a thread to discuss potential changes here in the arcade side, but I am not sure how quickly that will move. We may still want to try to do something on our end to unblock our dependency flow.

@rolfbjarne
Copy link
Member

I am not sure how quickly that will move.

I created a PR to hopefully make this happen: dotnet/arcade#15023

@pjcollins
Copy link
Member Author

pjcollins commented Aug 22, 2024

Thanks for the pending arcade fix! I've updated the PR and description to capture the feedback and note that the metadata is now optional. We can merge this to remove it for now, and restore it once we have an update to arcade if that sounds like a reasonable plan. Note that the + portion of the metadata string will need to be moved to the variable declaration when restored, which should hopefully not be problematic.

@pjcollins pjcollins requested a review from rolfbjarne August 22, 2024 13:11
@pjcollins pjcollins changed the title [build] Update package metadata [build] Make package metadata optional Aug 22, 2024
@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [CI Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Big Sur (11) passed 💻

All tests on macOS M1 - Mac Big Sur (11) passed.

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 168 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 6 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [CI Build] Windows Integration Tests failed ❌

❌ Failed ❌

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

2 similar comments
@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [CI Build] Windows Integration Tests failed ❌

❌ Failed ❌

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [CI Build] Windows Integration Tests failed ❌

❌ Failed ❌

Pipeline on Agent
Hash: 2851cf71b5ccbdd5a262929cf19b7494f88b2fd3 [PR build]

@dalexsoto
Copy link
Member

/sudo backport release/8.0.1xx-xcode15.4

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/8.0.1xx-xcode15.4 Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10097850 for more details.

@pjcollins pjcollins merged commit 92092f6 into main Aug 23, 2024
29 checks passed
@pjcollins pjcollins deleted the dev/pjc/pack-repo-meta branch August 23, 2024 16:19
@pjcollins
Copy link
Member Author

/sudo backport release/9.0.1xx-rc1

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/9.0.1xx-rc1 Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10098330 for more details.

dalexsoto pushed a commit that referenced this pull request Aug 24, 2024
Looking at the package explorer for recent .NET 9 preview 7 builds I
noticed that other packages do not include sha metadata in the version
element. Compare the following by clicking "View Metadata source":


https://nuget.info/packages/Microsoft.iOS.Ref.net9.0_17.5/17.5.9231-net9-p7

https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.ios-arm64/9.0.0-preview.7.24405.7
https://nuget.info/packages/Microsoft.Maui.Sdk/9.0.0-preview.7.24407.4

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
[arcade publishing tooling][0] which expects these to be in sync.

While waiting for a fix in arcade, we can remove this metadata to
restore dependency flow. It can be restored in the future by setting
the following:

    NUGET_BUILD_METADATA=+sha.$(CURRENT_HASH)

The icon and other package metadata have been updated to better match
other .NET packages.

[0]:
https://github.com/dotnet/arcade/blob/b4f4d40741f161e2c0d96c19c51a4013850ef65f/src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs


Backport of #21081

---------

Co-authored-by: Peter Collins <[email protected]>
Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
dalexsoto pushed a commit that referenced this pull request Aug 24, 2024
…21105)

Looking at the package explorer for recent .NET 9 preview 7 builds I
noticed that other packages do not include sha metadata in the version
element. Compare the following by clicking "View Metadata source":


https://nuget.info/packages/Microsoft.iOS.Ref.net9.0_17.5/17.5.9231-net9-p7

https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.ios-arm64/9.0.0-preview.7.24405.7
https://nuget.info/packages/Microsoft.Maui.Sdk/9.0.0-preview.7.24407.4

The sha metadata inclusion in the version element but not the .nupkg
name also causes conflicts with the latest version of the
[arcade publishing tooling][0] which expects these to be in sync.

While waiting for a fix in arcade, we can remove this metadata to
restore dependency flow. It can be restored in the future by setting
the following:

    NUGET_BUILD_METADATA=+sha.$(CURRENT_HASH)

The icon and other package metadata have been updated to better match
other .NET packages.

[0]:
https://github.com/dotnet/arcade/blob/b4f4d40741f161e2c0d96c19c51a4013850ef65f/src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs


Backport of #21081

---------

Co-authored-by: Peter Collins <[email protected]>
Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants