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.

Sign up for GitHub

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

Jump to bottom

[dotnet] Accept invalid runtime identifiers for Restore. #15357

Merged
merged 4 commits into from
Jul 12, 2022
Merged

[dotnet] Accept invalid runtime identifiers for Restore. #15357

merged 4 commits into from
Jul 12, 2022

Conversation

rolfbjarne
Copy link
Member

Validating that projects are only restored using valid runtime identifiers
have turned out to be an interesting rabbit hole.

Nobody seems to dispute the fact that it's a correct validation, the problem
is that it apparently turns out quite complicated to not do the wrong thing
for projects with multiple target frameworks.

In some scenarios apparently the Restore target might want to restore all
target frameworks, which breaks down if whatever the user wants to do requires
setting a runtime identifier, because then that runtime identifier is set for
all target frameworks.

So for the sake of simplicity, we're going to try removing this validation for
the Restore target (we're keeping it for the Build target).

There are thus two potential scenarios:

  1. The Restore succeeds using invalid runtime identifiers. This shouldn't
    affect valid builds (since those should be completely separate due to using
    different runtime identifiers).
  2. Something else breaks. At worst the user will be given a less
    comprehensible error message. Time will tell if this is better or worse
    than the current experience.

Ref: NuGet/Home#11653
Ref: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1558247

@rolfbjarne
[dotnet] Accept invalid runtime identifiers for Restore.
ff62526 
Validating that projects are only restored using valid runtime identifiers
have turned out to be an interesting rabbit hole.

Nobody seems to dispute the fact that it's a correct validation, the problem
is that it apparently turns out quite complicated to not do the wrong thing
for projects with multiple target frameworks.

In some scenarios apparently the Restore target might want to restore all
target frameworks, which breaks down if whatever the user wants to do requires
setting a runtime identifier, because then that runtime identifier is set for
all target frameworks.

So for the sake of simplicity, we're going to try removing this validation for
the Restore target (we're keeping it for the Build target).

There are thus two potential scenarios:

1. The Restore succeeds using invalid runtime identifiers. This shouldn't
   affect valid builds (since those should be completely separate due to using
   different runtime identifiers).
2. Something else breaks. At worst the user will be given a less
   comprehensible error message. Time will tell if this is better or worse
   than the current experience.

Ref: NuGet/Home#11653
Ref: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1558247
@rolfbjarne rolfbjarne added the not-notes-worthy Ignore for release notes label Jun 27, 2022
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
dalexsoto
dalexsoto approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

Not a fan but 👍

@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
3 similar comments
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [PR Build] Build failed 🔥

Build failed for the job 'Detect API changes'

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire

Pipeline on Agent
Hash: cd336ab51ca76d97c9f443ce0c4084fa03f5bdb5 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1094.Monterey'
Hash: 7ba8b55d887d95064c249b5debbe23464d446f1a [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 7ba8b55d887d95064c249b5debbe23464d446f1a [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • xammac_tests
  • monotouch-test

Pipeline on Agent
Hash: 7ba8b55d887d95064c249b5debbe23464d446f1a [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 223 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 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 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 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: [PR build]

@rolfbjarne rolfbjarne merged commit f7772d4 into xamarin:main Jul 12, 2022
@rolfbjarne rolfbjarne deleted the dotnet-accept-invalid-rids-for-restore branch July 12, 2022 07:37
@rolfbjarne
Copy link
Member Author

/sudo backport release/6.0.3xx

@rolfbjarne
Copy link
Member Author

/sudo backport release/6.0.4xx

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/6.0.3xx Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch release/6.0.4xx Created! The magic is happening here

This was referenced Jul 13, 2022
Merged
Merged
@vs-mobiletools-engineering-service2
Copy link
Collaborator

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

@vs-mobiletools-engineering-service2
Copy link
Collaborator

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

rolfbjarne added a commit that referenced this pull request Jul 14, 2022
@vs-mobiletools-engineering-service2 @rolfbjarne
[release/6.0.4xx] [dotnet] Accept invalid runtime identifiers for Res…
d3e28b2 
…tore. (#15491)

Validating that projects are only restored using valid runtime identifiers
have turned out to be an interesting rabbit hole.

Nobody seems to dispute the fact that it's a correct validation, the problem
is that it apparently turns out quite complicated to not do the wrong thing
for projects with multiple target frameworks.

In some scenarios apparently the Restore target might want to restore all
target frameworks, which breaks down if whatever the user wants to do requires
setting a runtime identifier, because then that runtime identifier is set for
all target frameworks.

So for the sake of simplicity, we're going to try removing this validation for
the Restore target (we're keeping it for the Build target).

There are thus two potential scenarios:

1. The Restore succeeds using invalid runtime identifiers. This shouldn't
   affect valid builds (since those should be completely separate due to using
   different runtime identifiers).
2. Something else breaks. At worst the user will be given a less
   comprehensible error message. Time will tell if this is better or worse
   than the current experience.

Ref: NuGet/Home#11653
Ref: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1558247


Backport of #15357

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
rolfbjarne added a commit that referenced this pull request Jul 14, 2022
@vs-mobiletools-engineering-service2 @rolfbjarne
[release/6.0.3xx] [dotnet] Accept invalid runtime identifiers for Res…
c23dbe9 
…tore. (#15490)

Validating that projects are only restored using valid runtime identifiers
have turned out to be an interesting rabbit hole.

Nobody seems to dispute the fact that it's a correct validation, the problem
is that it apparently turns out quite complicated to not do the wrong thing
for projects with multiple target frameworks.

In some scenarios apparently the Restore target might want to restore all
target frameworks, which breaks down if whatever the user wants to do requires
setting a runtime identifier, because then that runtime identifier is set for
all target frameworks.

So for the sake of simplicity, we're going to try removing this validation for
the Restore target (we're keeping it for the Build target).

There are thus two potential scenarios:

1. The Restore succeeds using invalid runtime identifiers. This shouldn't
   affect valid builds (since those should be completely separate due to using
   different runtime identifiers).
2. Something else breaks. At worst the user will be given a less
   comprehensible error message. Time will tell if this is better or worse
   than the current experience.

Ref: NuGet/Home#11653
Ref: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1558247


Backport of #15357

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
Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes

@sbanni sbanni sbanni approved these changes

@mandel-macaque mandel-macaque mandel-macaque approved these changes

@chamons chamons chamons approved these changes

@stephen-hawley stephen-hawley stephen-hawley approved these changes

@tj-devel709 tj-devel709 tj-devel709 approved these changes

Assignees
No one assigned
Labels
not-notes-worthy Ignore for release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rolfbjarne @vs-mobiletools-engineering-service2 @dalexsoto @sbanni @mandel-macaque @chamons @stephen-hawley @tj-devel709