Feature: Do not reuse different architecture version of temporary APE Loader on Darwin #1410
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Bence, thank you for your contribution! It's an interesting approach that I think may work (although I'm also not sure if it breaks anything for others). As a first time contributor, could you check instructions in CONTRIBUTING.md? |
|
Well, sure, I've started, and then got distracted after the send email part. Will move on now. UPDATE: Okay, so formatting is done now. Imho it makes code harder to read here but this is the case with auto-formatters usually... Not sure if there is anything else I missed? |
BenceSzalai
commented
May 1, 2025
Comment on lines
555
to
568
| if ((p = getenv("TMPDIR"))) { | ||
| UnveilIfExists( | ||
| __join_paths(buf, sizeof(buf), p, ".ape-" APE_VERSION_STR), "rx"); | ||
| UnveilIfExists(__join_paths(buf, sizeof(buf), p, | ||
| ".ape-" APE_VERSION_STR GetXnuSuffix()), | ||
| "rx"); | ||
| } | ||
| if ((p = getenv("HOME"))) { | ||
| UnveilIfExists( | ||
| __join_paths(buf, sizeof(buf), p, ".ape-" APE_VERSION_STR), "rx"); | ||
| UnveilIfExists(__join_paths(buf, sizeof(buf), p, | ||
| ".ape-" APE_VERSION_STR GetXnuSuffix()), | ||
| "rx"); | ||
| } |
There was a problem hiding this comment.
Probably these could be wrapped in a condition as well to only run on xnu, but I don't know what / if there is an appropriate check available in this context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Addresses #1409
So I thought I'll attempt to address the issue which caused me large headache few days ago.
My goal with this PR is to create the temporary APE loader in different files per architecture on Apple Silicon Mac:
${TMPDIR:-${HOME:-.}}/.ape-1.10-x86_64${TMPDIR:-${HOME:-.}}/.ape-1.10-arm64+${TMPDIR:-${HOME:-.}}/.ape-1.10-arm64.cThis way one can use Cosmo apps with both Rosetta and native ARM mode, even at the same time (without installing APE loader and manually setting up different versions and manipulating the PATH per architecture).
What I actually don't understand is why it appears that the same logic is implemented twice:
The code from
ape/ape.S:613andtool/build/apelink.c:2019seems to do the same thing.I have attempted to make sure they behave the same in my PR as well, but actually I don't know what
ape/ape.Sdoes or when it is used, so I could not test if they indeed are fully compatible after my changes.As far as I could, I tested it and it worked nice in my environment. One of the big questions is if it breaks anything for others?