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

milo/support aarch64 unknown gnu #123

Merged
merged 1 commit into from
Jan 29, 2023
Merged

Conversation

Milo123459
Copy link
Contributor

@Milo123459 Milo123459 commented Jan 13, 2023

This allows cargo-quickbuild to build for aarch64-unknown-linux-gnu.

Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

For the linter error:

build-version.sh Outdated Show resolved Hide resolved
zig-aarch64.sh Outdated Show resolved Hide resolved
build-version.sh Outdated Show resolved Hide resolved
build-version.sh Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Member

Could you be hitting ziglang/zig#4911 ?

@Milo123459
Copy link
Contributor Author

I think so.

@Milo123459
Copy link
Contributor Author

getting a new error this time. cc @NobodyXu - any ideas?

@Milo123459
Copy link
Contributor Author

right - now im trying to make it use the NDK version of ld. currently, when i move ld to /usr/bin, the shell doesn't pick up that there is something in that directory called ld and now errors out because it cannot find it. it turns out that the sudo mv isn't actually moving the file, i think

@Milo123459
Copy link
Contributor Author

cc @alsuren - any ideas

@Milo123459 Milo123459 force-pushed the milo/support-aarch64-unknown-gnu branch 2 times, most recently from c0e47d8 to 70c09fd Compare January 28, 2023 22:34
Copy link
Collaborator

@alsuren alsuren left a comment

Choose a reason for hiding this comment

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

Please give your squashed commit a better name, and consider my suggestions (it may be that you already tried them)

I would quite like to try one of these binaries in docker on my m1 mac. Let's ship it and get it building stuff :D

zig-aarch64.sh Outdated Show resolved Hide resolved
build-version.sh Outdated Show resolved Hide resolved
@Milo123459
Copy link
Contributor Author

i can't merge it because there are some review changes that haven't been addressed (even though they have). @alsuren could you merge it for me? thanks

@alsuren
Copy link
Collaborator

alsuren commented Jan 28, 2023

The "request changes" review is blocking the merge. I tend to avoid using that button because it feels impolite (I always tend to leave feedback as "comment"). I hadn't realised that it also blocks merging in the presence of other approvers :-(.

If you want to smoke test it before we merge to main, push it to the actions branch and it will kick off a one-off cronjob using your code.

@Milo123459
Copy link
Contributor Author

alright, i'll push it! fingers crossed

@Milo123459
Copy link
Contributor Author

seems to have worked! should i delete the actions branch?

@Milo123459
Copy link
Contributor Author

nvm, it's failing because it's using a weird version of the branch causing it to not have access to a file. in practice, this won't happen if this is merged and the cron just runs.

ps @alsuren - can you not override the requested changes thing and merge it?

@alsuren
Copy link
Collaborator

alsuren commented Jan 28, 2023

ps @alsuren - can you not override the requested changes thing and merge it?

Oh hey: there is a "dismiss review" item in the menu.

I still think that https://github.com/cargo-bins/cargo-quickinstall/actions/runs/4033938309/jobs/6934774496#step:4:89 (

error: linker `/home/runner/work/cargo-quickinstall/cargo-quickinstall/zig-aarch64.sh` not found

) is a problem that we're also going to see on main. Let me have a quick dig about.

build-version.sh Outdated Show resolved Hide resolved
@alsuren
Copy link
Collaborator

alsuren commented Jan 29, 2023

I just tested it out in arm docker, and cargo-quickinstall cargo-quickinstall now works (once I installed curl). Good work, and thanks for sticking with it.

Glancing at https://github.com/cargo-bins/cargo-quickinstall/pull/123/commits still doesn't tell me anything about what this PR is about, so it's likely to cause problems for anyone doing archaeology later (although they will have enough problems unpicking my commits, so they might not notice yours).

Otherwise, you have an approval and I think you have merge access, so ship it.

Now it's well past my bedtime. I might do some more quickinstall hacking tomorrow though.

@Milo123459
Copy link
Contributor Author

Woo! I'll squash my commits again. Just testing the actions branch then I'll give it a merge!

…ickinstall to build binaries for the aarch64-unknown-linux-gnu target, using Zig as a linker for cross-compilation.
@Milo123459 Milo123459 force-pushed the milo/support-aarch64-unknown-gnu branch from 2e2647d to 2e13042 Compare January 29, 2023 00:36
@Milo123459 Milo123459 merged commit 8e4ea52 into main Jan 29, 2023
@Milo123459 Milo123459 deleted the milo/support-aarch64-unknown-gnu branch January 29, 2023 00:39
@Milo123459
Copy link
Contributor Author

Seeing some errors @alsuren 😬 https://github.com/cargo-bins/cargo-quickinstall/actions/runs/4034450545/jobs/6935671373#step:5:101

PS: I ran cargo quickinstall cargo-binstall on my Raspberry Pi, that has the aarch64-unknown-linux-gnu target, but a workflow run was never created for it.

@NobodyXu
Copy link
Member

This error is caused by my PR #140, will fix it

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.

3 participants