Skip to content

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Aug 17, 2022

Opening this as an alternative to reverting the PublishAot changes in #74036.
The intention was to default PublishAot to true when using the package reference, not when using the ILCompiler that ships with the SDK. These props get imported on both paths - I think it was just missing a check.

@sbomer sbomer requested a review from LakshanF August 17, 2022 01:56
@ghost ghost assigned sbomer Aug 17, 2022
Copy link
Contributor

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky
Copy link
Member

/backport to release/7.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2872323345

@sbomer
Copy link
Member Author

sbomer commented Aug 17, 2022

I did some amount of validation locally on a selection of the SDK tests that were failing in dotnet/sdk#27149 and am seeing them pass with this change.

@radical
Copy link
Member

radical commented Aug 17, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Aug 17, 2022

Also, ILCompiler* sdk shouldn't be imported for browser-wasm.

@MichalStrehovsky
Copy link
Member

Also, ILCompiler* sdk shouldn't be imported for browser-wasm.

Props files don't know what we're going to target because they're included before users csproj that has those details. WASM builds include Windows targeting-related-props files as well; it's fine. They're not supposed to have side effects like this bug had, just set up the environment in a user-overridable way.

@lewing lewing merged commit 6107588 into dotnet:main Aug 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2022
@sbomer sbomer deleted the fixPublishAotSetting branch November 3, 2023 18:37
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.

5 participants