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.

Sign up for GitHub

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

Fix arch detection x86 64 #56

Merged
merged 3 commits into from
Aug 5, 2024
+5 −5
Merged

Fix arch detection x86 64 #56

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d0b2a
Add protection against escaping issues
TheAssassin Aug 5, 2024
f3a3666
Fix architecture detection for x86_64
TheAssassin Aug 5, 2024
d14d424
Merge branch 'main' into fix-arch-detection-x86_64
probonopd Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions scripts/build-in-container.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ ls -lh runtime
# append architecture prefix
# since uname gives the kernel architecture but we need the userland architecture, we check /bin/bash
# all we have to do is convert uname's expected output to AppImage's semi-official suffix style
runtime=$(file -L /bin/bash)
runtime="$(file -L /bin/bash)"

if [[ $runtime =~ 80386 ]]; then
if [[ "$runtime" =~ 80386 ]]; then
architecture=i686
elif [[ $runtime =~ aarch64 ]]; then
elif [[ "$runtime" =~ aarch64 ]]; then
architecture=aarch64
elif [[ $runtime =~ EABI5 ]]; then
elif [[ "$runtime" =~ EABI5 ]]; then
architecture=armhf
elif [[ $runtime =~ x86_64 ]]; then
elif [[ "$runtime" =~ x86-64 ]]; then
architecture=x86_64
Comment on lines +39 to 46
Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, a case statement would have made more sense. You can find a few examples in this repository.

Copy link
Member

Choose a reason for hiding this comment

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

It's a matter of taste. I find if/elif/else more explicit and easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how it's more explicit, it just requires more syntax. And case allows for matching more than one value more easily and more explicitly. Using regular expressions is a lot more complex than using fname style wildcard matching. Either way, as long as it works...

Copy link
Member

Choose a reason for hiding this comment

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

The real question for me is, why do we have to do this at all, why can't we just use

type2-runtime/.github/workflows/build.yaml

Lines 29 to 34 in 4799cde

matrix:
include:
- appimage_arch: i686
- appimage_arch: x86_64
- appimage_arch: armhf
- appimage_arch: aarch64

I can only suspect that it has to do with the fact that environment variables are not visible inside Docker by default? (Something I don't like about Docker)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can only suspect that it has to do with the fact that environment variables are not visible inside Docker by default?

Nope. Not at all...?

How would you build otherwise? You need to tell GitHub what you want to accomplish. That's what the matrix is there for.

Copy link
Member

@probonopd probonopd Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, so why don't we use those values but instead define them in the bash script by inspecting /bin/bash? Would solve

Copy link
Member Author

Choose a reason for hiding this comment

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

Which "those values"? You mean you'd prefer a solution that provides this value explicitly from the outside? Well, that's a tad more annoying because the data can actually be gathered from the environment, after all. I'd rather have a mapping in the script than have users provide it externally.

If it ever becomes too annoying, we can still rework it.

else
echo "Unsupported architecture: ${runtime#* }"
Expand Down
Loading