Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Android: add cross-compilation configuration file flag #3403
Android: add cross-compilation configuration file flag #3403
Changes from all commits
5fa1abc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is correct; there are a number of targets that may be used, can you just invert the condition here instead?
Yes, it isn't great, but leaves the behaviour the same while adding support for your use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is right, see the similar logic at the end of the script. Basically, Mishal only added support for cross-compiling SPM for macOS arm64 and he explicitly spelled that out below, but didn't bother here: I went ahead and made it explicit here too, now that I added Android also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WFM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this works and allows me to remove the extra triple flag. I just realized that the destination.json is just a JSON file, so we can parse it to get the target triple.
@abertelrud, if you agree with this change, just let me know and I will squash it into the first commit and update the commit message and we can get this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @buttaface, simplifying this down to get the information from the JSON instead of the having to pass another flag is a good improvement. I still want to hear what @neonichu and @tomerd think but this looks good given how things currently work. I will still want to work with @shahmishal to see if we can simplify the conceptual model around building for various platforms and architectures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will squash this commit and update the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the only use of this
targets
parameter was removed below, as the PD libraries no longer need to be built by CMake, I removed it altogether.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this is existing code, but as I read it, this will only do the right thing if I'm building for Apple Silicon on an Intel machine. Building a universal toolchain on an Apple Silicon machine doesn't seem to be supported, since
built_target
won't bex86_64-apple-macosx
. Am I misreading this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahmishal Do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as I noted in a review comment for this pull earlier, only cross-compiling from mac x86_64 to mac arm64 is currently supported. It will error here if you go the other way.