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

XXX: Fix macOS cross-builds #1510

Closed
wants to merge 1 commit into from

Conversation

jlduran
Copy link
Member

@jlduran jlduran commented Nov 3, 2024

Commit cb5e41b also broke macOS cross-builds. The error message thrown (abbreviated) is:

ld: unknown file type in '/Users/runner/.../tmp/legacy/usr/lib/libcrypt.a'

The file libcrypt.a contains the result of the sed manipulation from the libcrypt.ald target:

INPUT(-lcrypt_real -lmd)

This is because the target install-libcrypt.a essentially just copies libcrypt.ald to libcrypt.a

That is what is evidenced in this Pull Request, by copying libcrypt_real.a to libcrypt.a. With libncurses, this does not happen.

The reasons why it fails only on macOS are not related, and will hopefully be discussed more extensively in a separate pull request.

Commit cb5e41b also broke macOS cross-builds. The error message
thrown (abbreviated) is:

    ld: unknown file type in '/Users/runner/.../tmp/legacy/usr/lib/libcrypt.a'

The file `libcrypt.a` contains the result of the `sed` manipulation from
the `libcrypt.ald` target:

    INPUT(-lcrypt_real -lmd)

This is because the target `install-libcrypt.a` essentially just copies
`libcrypt.ald` to `libcrypt.a`

That is what is evidenced in this Pull Request, by copying
`libcrypt_real.a` to `libcrypt.a`.  With libncurses, this does not
happen.

The reasons why it fails only on macOS are not related, and will
hopefully be discussed more extensively in a separate pull request.
@jlduran
Copy link
Member Author

jlduran commented Nov 3, 2024

Copy link
Contributor

@clausecker clausecker left a comment

Choose a reason for hiding this comment

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

The fix breaks the whole purpose of the linker script: it exists so consumers who try to link -lcrypt also pull in -lmd, which is now a dependency of libcrypt. Replacing the script with just libcrypt_real.a destroys this mechanism. Unfortunately, macOS ld does not support libraries being linker scripts, which is why the build fails.

We have discussed multiple fix ideas for this on IRC, but I have so far not been able to make any of them work:

  • we could just put the relevant or all libmd.a objects into libcrypt.a (probably possible, but we don't have a tool to do that easily)
  • we could change things so the cross-tools build uses libcrypt_real instead of libcrypt (would probably work, but when I tried to implement this, it didn't work)
  • we could drop the hack and just require static consumers of libcrypt to link in libmd, too (cleanest solution, but comes with some compatibility impact)

However, your change is not a correct fix and should not be merged (reject). If you find a way to do one of the other solutions, especially the second one, that would be much appreciated.

@jlduran
Copy link
Member Author

jlduran commented Nov 3, 2024

We have discussed multiple fix ideas for this on IRC, but I have so far not been able to make any of them work:

Thank you for this explanation. The purpose of this pull request was not to present it as a fix, but the start of a possible solution. I appreciate sharing your preferred options.

  • we could change things so the cross-tools build uses libcrypt_real instead of libcrypt (would probably work, but when I tried to implement this, it didn't work)
    If you find a way to do one of the other solutions, especially the second one, that would be much appreciated.

I'll try to focus on this one. Thank you!

@jlduran jlduran closed this Nov 3, 2024
@jlduran jlduran deleted the fix-macos-cross-builds branch November 3, 2024 20:27
@clausecker
Copy link
Contributor

Feel free to reuse this PR should a better solution obtain.

@clausecker
Copy link
Contributor

This issue has since been worked around by @bapt in 4d69286. Further, more systematic fixes are planned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants