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

Discourage using explicit package references to ILCompiler to publish #74591

Merged
merged 6 commits into from
Aug 29, 2022

Conversation

LakshanF
Copy link
Member

@LakshanF LakshanF commented Aug 25, 2022

Fixes #74387 and dotnet/sdk#27239

Publishing a NativeAOT application is supported via the SDK and the 'old' unsupported way that we started via adding explicit package reference is discouraged. The explicit support is for very narrow scenarios like getting the latest fix on native AOT issues before the SDK exposes the fix via its official ILCompiler packages.

We also no longer support publishing an AOT application if the SDK property, PublishAOT, is not set and only the explicit package reference exists. We allowed this behavior (no PublishAOT specified) early in 7.0 when SDK support was not enabled but do not want to continue to support the old way now. SDK path is the only we way want to support except for the narrow scenario described above where PublishAOT is still required for that scenario to work.

@jkotas
Copy link
Member

jkotas commented Aug 25, 2022

I am wondering whether we should kill the feature that automatically turns on PublishAot when the package is explicitly references. It would steer people towards just using the PublishAot property. Thoughts?

@LakshanF
Copy link
Member Author

I am wondering whether we should kill the feature that automatically turns on PublishAot when the package is explicitly references. It would steer people towards just using the PublishAot property. Thoughts?

I like that that but that would break any existing usage of that pattern from 7.0 onwards. The warning text in this change would only be triggered when PublishAOT is set to true (and an explicit reference exist). Is that ok?

@jkotas
Copy link
Member

jkotas commented Aug 26, 2022

The warning text in this change would only be triggered when PublishAOT is set to true (and an explicit reference exist). Is that ok?

I think it is ok. People are going to quickly learn that the package reference alone does not do anything.

If we want to do this, it would be nice to get this into 7.0 to avoid 7.0->8.0 breaking change around what the package reference does.

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM.

@LakshanF
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@LakshanF LakshanF changed the title Show a warning when an explicit package reference to ILCompiler is added Discourage using explicit package references to ILCompiler to publish Aug 27, 2022
@LakshanF
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2948185603

@LakshanF LakshanF merged commit 1c67528 into dotnet:main Aug 29, 2022
@LakshanF LakshanF deleted the AotSDKDocs branch August 29, 2022 11:36
@github-actions
Copy link
Contributor

@LakshanF an error occurred while backporting to release/7.0, please check the run log for details!

Error: @LakshanF is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=LakshanF

@LakshanF
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2950238449

@github-actions
Copy link
Contributor

@LakshanF an error occurred while backporting to release/7.0, please check the run log for details!

Error: @LakshanF is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=LakshanF

@danmoseley
Copy link
Member

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2950265406

@github-actions
Copy link
Contributor

@danmoseley backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Show a warning when an explicit package reference to ILCompiler is added
Applying: Apply suggestions from code review
Applying: Fix issue 27239
Applying: Apply suggestions from code review
Applying: exclude the warning for direct targets file invoke cases
Using index info to reconstruct a base tree...
M	src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
CONFLICT (content): Merge conflict in src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 exclude the warning for direct targets file invoke cases
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

LakshanF added a commit to LakshanF/runtime that referenced this pull request Aug 29, 2022
…dotnet#74591)

* Show a warning when an explicit package reference to ILCompiler is added

* Apply suggestions from code review

Co-authored-by: Jan Kotas <[email protected]>

* Fix issue 27239

* Apply suggestions from code review

Co-authored-by: Sven Boemer <[email protected]>

* exclude the warning for direct targets file invoke cases

Co-authored-by: Jan Kotas <[email protected]>
Co-authored-by: Sven Boemer <[email protected]>
carlossanlop pushed a commit that referenced this pull request Aug 30, 2022
…#74591) (#74769)

* Show a warning when an explicit package reference to ILCompiler is added

* Apply suggestions from code review

Co-authored-by: Jan Kotas <[email protected]>

* Fix issue 27239

* Apply suggestions from code review

Co-authored-by: Sven Boemer <[email protected]>

* exclude the warning for direct targets file invoke cases

Co-authored-by: Jan Kotas <[email protected]>
Co-authored-by: Sven Boemer <[email protected]>

Co-authored-by: Jan Kotas <[email protected]>
Co-authored-by: Sven Boemer <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 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.

[NativeAOT] Fail if a ILCompiler package runtime major version is different from target runtime
4 participants