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

Fix arch detection x86 64 #56

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Fix arch detection x86 64 #56

merged 3 commits into from
Aug 5, 2024

Conversation

TheAssassin
Copy link
Member

Related to #55.

@TheAssassin TheAssassin requested a review from probonopd August 5, 2024 18:52
Copy link

github-actions bot commented Aug 5, 2024

Build for testing:
artifacts
Use at your own risk.

@probonopd probonopd enabled auto-merge (squash) August 5, 2024 20:03
@probonopd probonopd merged commit 4799cde into main Aug 5, 2024
18 checks passed
Comment on lines +39 to 46
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
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

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

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.

@probonopd probonopd deleted the fix-arch-detection-x86_64 branch August 5, 2024 20:15
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.

2 participants