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

Refactor cross compile assistance #769

Merged
merged 28 commits into from
Feb 8, 2024
Merged

Conversation

runesoerensen
Copy link
Contributor

@runesoerensen runesoerensen commented Feb 1, 2024

We currently support targeting x86_64-unknown-linux-musl and aarch64-unknown-linux-musl on macOS and Linux, but the cross compile assistance provided is inconsistent depending on the host's CPU architecture. This refactoring aims to improve that across all supported target_triple and host OS/architecture combinations by providing helpful assistance for supported scenarios.

It also:

  • Drops support for a gcc binary (x86_64-linux-musl-gcc) installed by a previously recommended macOS toolchain. Since the current toolchain has been recommended for nearly 2 years it's probably safe to assume (or fair to require) that users have update to the recommended toolchain.
    The brew formula/revision has also been bumped, so the missing dependencies mentioned here Update cross-compile assistance for Apple silicon #312 (comment) are now included

This implementation draft factors in the host/env architecture when generating assistance help text and/or cargo environment variables for the target triple.

It drops support for the `x86_64-linux-musl-gcc` binary name on macOS (safe to assume users have migrated and/or provide assistance for currently supported toolchain).
This should be moved to shared template.

Instructions/required packages also needs to be tested further for each host OS (musl-tools and other packages may be required)
@Malax Malax self-requested a review February 1, 2024 17:14
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 🎉

I gave this a skim-read to provide some early feedback since this is one of your first PRs around here. I understand that this is a draft still.

Apart from the mostly minor code comments, I was missing some context due to the fact there is not PR description and the title being very vague (what does it fix, is there an existing issue somewhere?) Again, this is a draft PR and I write my descriptions at the very end as well. If that was to come later - ignore my comment! :)

There also seem to be some slight different in packages recommended. Have you verified those on the respective systems? For example, libc6-dev-arm64-cross is no longer mentioned. It's possible it is not needed, but from the information I have it could also be an oversight. :)

libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
@runesoerensen runesoerensen changed the title Fix cross compile assistance Refactor cross compile assistance Feb 2, 2024
This will likely work without treating `musl-gcc` differently than other gcc_binaries, but requires further testing to ensure that's the case.

The current implementation retains the logic prior to this pull request
No characters are used that would otherwise need escape
The help_text already includes a newline by the end
I previously removed libc6-dev-arm64-cross as it is installed by default (as a recommended package for the transient dependency) https://packages.ubuntu.com/jammy/gcc-11-aarch64-linux-gnu

Adding it back to the help text as it is not a hard dependency (and won't be installed if the `--no-install-recommends` flag is set)
@runesoerensen
Copy link
Contributor Author

runesoerensen commented Feb 2, 2024

Apart from the mostly minor code comments, I was missing some context due to the fact there is not PR description and the title being very vague (what does it fix, is there an existing issue somewhere?) Again, this is a draft PR and I write my descriptions at the very end as well. If that was to come later - ignore my comment! :)

I have added a more elaborate description now with links to relevant issues. Merging this pull request should close those issues, but let me know if it's better to unlink them and manually close the issues with reference to this pull request!

Should I perhaps add a CHANGELOG entry as well?

There also seem to be some slight different in packages recommended. Have you verified those on the respective systems? For example, libc6-dev-arm64-cross is no longer mentioned. It's possible it is not needed, but from the information I have it could also be an oversight. :)

I initially removed the libc6-dev-arm64-cross package from the help text since, during testing, it was installed along with g++-aarch64-linux-gnu - but only as a recommended package - c5a046b adds it back in and explains why. The following commit adds the equivalent packages for amd64 cross compilation from aarch64 Linux

I'm not sure if musl-tools is still necessary, but also added that to both help texts. I will test further, but overall it's probably best to keep the changes introduced by this pull request minimal (and improve on the help text in another PR).

@runesoerensen runesoerensen marked this pull request as ready for review February 5, 2024 04:36
@runesoerensen runesoerensen requested a review from a team as a code owner February 5, 2024 04:36
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Looking great! 👍🏻 Let's add a brief CHANGELOG entry and ship it!

@runesoerensen runesoerensen self-assigned this Feb 5, 2024
@runesoerensen runesoerensen added bug Something isn't working enhancement New feature or request libcnb-package labels Feb 5, 2024
@edmorley edmorley self-requested a review February 5, 2024 17:10
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for this! The implementation is both cleaner and more correct now :-)

I tested this manually locally on macOS by packaging the Python CNB (which includes C deps, such as ring to exercise the env vars being set), in the following scenarios:

  • the default target (x86_64-unknown-linux-musl) with and without the correct toolchain installed
  • --target aarch64-unknown-linux-musl with and without the correct toolchain installed

...and it worked great for all of those.

(Longer term we have #730 and #731 for exploring trying to test more scenarios in CI)

However, the binary used when the architectures don't match is still the *-gnu-gcc variant (as for instance aarch64-linux-musl-gcc is only available on aarch64 Linux (this requires further investigation, and a separate issue should be created for that)).

The intention of #724 was to be about the general case of "we probably shouldn't be using the non-MUSL gcc ever". This PR helps with a subset of cases, but not all (as you mentioned in the PR description). Rather than close #724 out, I've morphed the title to make it clearer that it's about the general case :-)

libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-package/src/cross_compile.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
runesoerensen and others added 2 commits February 8, 2024 09:52
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@runesoerensen runesoerensen merged commit 9d23107 into main Feb 8, 2024
4 checks passed
@runesoerensen runesoerensen deleted the fix-cross-compile-assistance branch February 8, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request libcnb-package
Projects
None yet
3 participants