search

taiki-e / upload-rust-binary-action

GitHub Action for building and uploading Rust binary to GitHub Releases.
Apache License 2.0
235 stars 20 forks source link

Fix strip on non-x86 targets #9

Closed taiki-e closed 2 years ago

taiki-e commented 2 years ago

There are dedicated stripping tools for architectures other than x86. If they are installed, use them; otherwise, skip strip.

Fixes #8

cc @Shadow53

Shadow53 commented 2 years ago

This looks like it would do the job! Thanks for being so quick about this.

A question, out of curiosity: why run *-strip --version instead of something like which *-strip or I think command -q strip (running the program vs just searching for it)?

Shadow53 commented 2 years ago

I am trying this branch in my CI to test, and I am still getting the same error:

 strip: Unable to recognise the format of the input file `hoard'

This is with the following Rust toolchains:

Notably, the following successfully built:

It looks like the successful ones were not stripped, as aarch64-linux-android does not match the pattern aarch64*-unknown-linux-* and I did not have the right cross-compilation tools installed for the other two.

I will update with another comment if I figure anything else out.

Shadow53 commented 2 years ago

Okay, I think I found the issue, You set strip="..." in the case statement, but around line 111 you call strip instead of $strip. Change it to the latter, and I think the issue I saw will go away.

taiki-e commented 2 years ago

Thanks @Shadow53! I've applied both fixes.

taiki-e commented 2 years ago

A question, out of curiosity: why run *-strip --version instead of something like which *-strip or I think command -q strip (running the program vs just searching for it)?

I tried to choose the one that worked most reliably because their subtle differences in behavior are complicated (https://github.com/koalaman/shellcheck/issues/1162), but in fact strip --version did not work on macos, so I switched to which in latest commit.

taiki-e commented 2 years ago

I've tested that these work (https://github.com/taiki-e/test/actions/runs/1286975387). merging