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

Improve support for linux/arm64/v8 on most Ubuntu-based images (part 2) #1630

Merged
merged 6 commits into from
Mar 3, 2025

Conversation

garasubo
Copy link
Contributor

This is a follow-up to #1597. Docker images for Arm64-based PCs has been added.

@garasubo garasubo requested a review from a team as a code owner February 19, 2025 05:41
@garasubo garasubo marked this pull request as draft February 19, 2025 05:41
@garasubo garasubo marked this pull request as ready for review February 19, 2025 07:20
ENV CROSS_SYSROOT=/usr/aarch64-linux-gnu

COPY apt-cross-essential.sh /
RUN TARGET_ARCH=arm64 TARGET_TRIPLE=aarch64-linux-gnu /apt-cross-essential.sh
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looking at this again makes me suspicious of this approach, it makes it harder to debug with a glance whats extra in the image. Id prefer it if the script was removed or if the wanted packages were explicitly passed as arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted these changes

Copy link
Member

Choose a reason for hiding this comment

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

I think there will be some problems with this, due to what was mentioned here

@Emilgardis
Copy link
Member

looks good, can you rebase the changes such that there's no merge commit?

@Emilgardis
Copy link
Member

/ci try

This comment has been minimized.

Copy link

Try run for comment

Failed Jobs

Successful Jobs

List

@Emilgardis
Copy link
Member

need to test with --platform arm64/v8 also

@ianks
Copy link
Contributor

ianks commented Feb 20, 2025

Hey thanks for doing this! Things have been crazy where I live so I've been short on time ❤️

@garasubo
Copy link
Contributor Author

@Emilgardis

need to test with --platform arm64/v8 also

Do you want me to change provided_images.rs and related codes to build images for arm64/v8 in this PR? I plan to change them in another PR.

@garasubo garasubo force-pushed the more-arm64-docker-builds branch from cc80331 to 9b8b766 Compare February 21, 2025 08:04
@garasubo garasubo force-pushed the more-arm64-docker-builds branch from 9b8b766 to a581024 Compare February 21, 2025 08:04
@garasubo
Copy link
Contributor Author

rebased the branch to remove merge commit from this PR.

@garasubo garasubo requested a review from Emilgardis February 21, 2025 08:19
@Emilgardis
Copy link
Member

@Emilgardis

need to test with --platform arm64/v8 also

Do you want me to change provided_images.rs and related codes to build images for arm64/v8 in this PR? I plan to change them in another PR.

If you think you can do it in this pr, feel free to do so. Take care of the comments i left in the previous pr about some ways to make it work.

@garasubo
Copy link
Contributor Author

@Emilgardis I caught up the discussion on the previous PR, and tried to change to enable to publish arm64 images, but it was harder than I expected. So, I'd like to merge this PR first, and then submit another PR. Could you review my change again?
I think I fixed the errors in the previous run and rebased the branch to remove merge commit from this PR.

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Going to try building some images myself, but overall it looks fine

Copy link
Member

Choose a reason for hiding this comment

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

This file is not used anymore, i think however it would be worth to incorporate the command checking it does since the previous pr mentioned it was needed to avoid issues with gfortran

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what kind of issues with gfortran the previous pr were referring to. But it looks all Dockerfiles have apt install gfortran-${TARGET_TRIPLE}. Is there anything else that should be done? Otherwise, I'll just remove this file from the pr.

@garasubo garasubo requested a review from Emilgardis March 3, 2025 08:11
@Emilgardis
Copy link
Member

going to merge this, still some work left that is needed, but we'll work on that

@Emilgardis Emilgardis changed the title Support linux/arm64/v8 for most Ubuntu-based images (part 2) Improve support for linux/arm64/v8 on most Ubuntu-based images (part 2) Mar 3, 2025
@Emilgardis Emilgardis added A-aarch64-host Area: ARMv8 hosts no-ci-targets PRs that do not affect or should skip any cross-compilation targets. labels Mar 3, 2025
@Emilgardis Emilgardis added this pull request to the merge queue Mar 3, 2025
Merged via the queue into cross-rs:main with commit 49338b1 Mar 3, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aarch64-host Area: ARMv8 hosts no-ci-targets PRs that do not affect or should skip any cross-compilation targets.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants