Skip to content

Conversation

directhex
Copy link
Contributor

No description provided.

@ghost ghost added the area-Build-mono label May 14, 2021
@directhex
Copy link
Contributor Author

directhex commented May 17, 2021

Running a private build since this affects packaging

https://dev.azure.com/dnceng/internal/_build/results?buildId=1141962&view=results

@directhex directhex marked this pull request as ready for review May 18, 2021 13:31
@directhex directhex requested a review from marek-safar as a code owner May 18, 2021 13:31
@directhex directhex added the os-maccatalyst MacCatalyst OS label May 18, 2021
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple nits that can be addressed in a follow-up PR.

#cmakedefine TARGET_TVOS 1

/* The JIT/AOT targets Mac Catalyst */
#cmakedefine TARGET_MACCAT 1
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we should call this TARGET_MACCATALYST so it's consistent with how we're using it everywhere else (can be done in a follow-up PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That came in with #51669, I'm innocent

- Browser_wasm
- tvOS_arm64
- iOS_arm64
- MacCatalyst_x64
Copy link
Member

Choose a reason for hiding this comment

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

nit: if I understand this correctly this job builds the AOT offsets for all archs, not just iOS_arm64 or MacCatalyst_x64 right? we should add a comment to explain it since this is quite confusing.

@directhex directhex merged commit 837605b into dotnet:main May 18, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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.

4 participants