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

Explicitly omit RID from crossgen2 binary path #78733

Merged
merged 1 commit into from
Nov 23, 2022
Merged

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Nov 22, 2022

@jkotas
Copy link
Member Author

jkotas commented Nov 22, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

LGTM, but be aware that this is still a changed behavior. RuntimeIdentifier will be set where it didn't used to be set before, even if it's not in the path. And I don't know if that has implications with how crossgen uses RuntimeIdentifiers. SelfContained and friends shouldn't allow you to build/publish if you don't have a RuntimeIdentifier unless there is some spicy customization going on here. If there's logic somewhere to allow that, it's unnecessary if the SDK infers a RuntimeIdentifier for you now, so it could be removed.

Also, FYI failure in musl arm64 appears unrelated:

   System.Net.Http.Tests.ContentDispositionHeaderValueTest.FileNameStar_NeedsEncoding_EncodedAndDecodedCorrectly [FAIL]
      Assert.Equal() Failure
                    ↓ (pos 4)
      Expected: FileÃName.bat
      Actual:   File?�Name.bat
                    ↑ (pos 4)

@jkotas
Copy link
Member Author

jkotas commented Nov 23, 2022

@nagilson Thank you!

SelfContained and friends shouldn't allow you to build/publish if you don't have a RuntimeIdentifier unless there is some spicy customization going on here

There is a lot of customization in dotnet/runtime build. My reason for fixing the issue using AppendRuntimeIdentifierToOutputPath is that other very similar project has this property set, and so it is unlikely to run into unexpected issues.

the SDK infers a RuntimeIdentifier for you now

I do not think that the SDK can infer RuntimeIdentifier for us. dotnet/runtime supports number of cross-building scenarios (e.g. running on linux-x64 and building for linux-arm64). The RID has to be explicitly specified for these cases.

failure in musl arm64 appears unrelated

Yes, it is unrelated (build-analysis has a link to the tracking issue).

@jkotas jkotas merged commit 563297d into dotnet:main Nov 23, 2022
@nagilson
Copy link
Member

nagilson commented Nov 23, 2022

I do not think that the SDK can infer RuntimeIdentifier for us

While the SDK might not be able to cleverly in this case, the breaking change is that it will try to use the machine RID if you have a property like SelfContained or PublishSingleFile, as normally these properties will fail your build if RuntimeIdentifier is not set.
UseCurrentRuntimeIdentifier would disable that. But I think the path decision was the right way to go, because we are also considering making net 8 apps RID specific by default, so then you'd need that and RuntimeSpecific = false.

dotnet/sdk#26028

@jkotas jkotas deleted the source-build branch November 23, 2022 21:34
@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2022
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.

[release/7.0.2xx] Runtime fails to generate native System.Private.CoreLib in source-build bootstrap
2 participants