Skip to content

Enable native AOT for non-portable crossgen2 #97536

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
Merged
jkotas merged 2 commits into from
Feb 2, 2024
Merged

Enable native AOT for non-portable crossgen2 #97536

jkotas merged 2 commits into from
Feb 2, 2024

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Jan 26, 2024

  • Add a breadcrumb for non-portable build into the Native AOT runtime pack
  • Use the breadcrumb to add system OpenSSL libs to the linker command line

Contributes to #66859

@ghost ghost assigned jkotas Jan 26, 2024
@jkotas jkotas requested review from jkoritzinsky and removed request for MichalStrehovsky January 26, 2024 05:38
@ghost
Copy link

ghost commented Jan 26, 2024

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Add a breadcrumb for non-portable build into the Native AOT runtime pack
  • Use the breadcrumb to add system OpenSSL libs to the linker command line

Contributes to #66859

Author: jkotas
Assignees: jkotas
Labels:

area-NativeAOT-coreclr
Milestone: -

@jkotas jkotas requested a review from agocke January 26, 2024 05:38
@jkotas
Enable native AOT for non-portable crossgen2
6a6caf3 
- Add a breadcrumb for non-portable build into the Native AOT runtime pack
- Use the breadcrumb to add system OpenSSL libs to the linker command
  line

Contributes to dotnet#66859
@jkotas jkotas force-pushed the non-portable-naot-build branch from 17e0835 to 6a6caf3 Compare January 26, 2024 05:42
@hoyosjs
Copy link
Member

hoyosjs commented Jan 26, 2024

Does it make sense to delete the breadcrumb if it's the portable build? Or error out on existence? That way you avoid potential issues of a stale breadcrumb. Also, is the targets file shipped? Or can the fine path be hoisted to a property that is shared?

This was referenced Jan 26, 2024
Closed
Open
@jkotas jkotas mentioned this pull request Jan 26, 2024
Closed
@jkotas
Copy link
Member Author

jkotas commented Jan 26, 2024

Does it make sense to delete the breadcrumb if it's the portable build?

I can delete the breadcrumb.

Are the incremental reliable when one flips /p:PortableBuild=true and /p:PortableBuild=true back and forth? I would expect that there are many subtle issues. I would not recommend anybody depending on incremental builds like this working.

@jkotas
Copy link
Member Author

jkotas commented Jan 26, 2024

Also, is the targets file shipped?

Yes, this is shipped in naot package.

@build-analysis build-analysis bot mentioned this pull request Jan 26, 2024
Open
3 tasks
@jkotas
Copy link
Member Author

jkotas commented Feb 2, 2024

@hoyosjs @agocke Could you please approve if it looks good?

@jkotas jkotas merged commit 968120b into dotnet:main Feb 2, 2024
@jkotas jkotas deleted the non-portable-naot-build branch February 2, 2024 14:31
agocke
agocke reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

jkotas added a commit to jkotas/runtime that referenced this pull request Feb 12, 2024
@jkotas
Partial revert "Enable native AOT for non-portable crossgen2 (dotnet#…
03ce398 
…97536)"
@jkotas jkotas mentioned this pull request Feb 12, 2024
Merged
jkotas added a commit that referenced this pull request Feb 12, 2024
@jkotas
Partial revert "Enable native AOT for non-portable crossgen2 (#97536)" (
2b3b182 
#98303)
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@agocke agocke agocke left review comments

@hoyosjs hoyosjs hoyosjs approved these changes

@jkoritzinsky jkoritzinsky Awaiting requested review from jkoritzinsky

+1 more reviewer

@tmds tmds tmds left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jkotas jkotas

Labels

area-NativeAOT-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jkotas @hoyosjs @agocke @tmds