Skip to content

Don't set PublishAot in SDK by default #74046

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 1 commit into from
Aug 17, 2022
Merged

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