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

CI: Add AppImage #1391

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Samueru-sama
Copy link

@Samueru-sama Samueru-sama commented Nov 2, 2024

This PR introduces the generation of an AppImage.

Some context here

The produced appimage uses the static appimage runtime and bundles all the libraries and its own ld-musl.so, so it should work on any linux system from the last 15 years.

Also I'm creating some dummy icon and .desktop entry, it would be nice if dunst gets its official .desktop and icon.


Also while the AppImage that I produce from the last stable build works perfectly, I just tested the one produced here which is a git build and it does have some issues finding the icons? and also the notification window is bigger for some reason, not sure if this is a known issue or an issue that would only affect the appimage for recent builds? UPDATE: Fixed

@Samueru-sama
Copy link
Author

Also while the AppImage that I produce from the last stable build works perfectly, I just tested the one produced here which is a git build and it does have some issues finding the icons? and also the notification window is bigger for some reason, not sure if this is a known issue or an issue that would only affect the appimage for recent builds?

Just quickly installed the dunst -git aur package to check, and yes I have the same issue with it, which I means I found a bug that I likely need to report if hasn't been already lol

@bynect
Copy link
Member

bynect commented Nov 2, 2024

This PR introduces the generation of an AppImage.

Some context here

The produced appimage uses the static appimage runtime and bundles all the libraries and its own ld-musl.so, so it should work on any linux system from the last 15 years.

Also I'm creating some dummy icon and .desktop entry, it would be nice if dunst gets its official .desktop and icon.


Also while the AppImage that I produce from the last stable build works perfectly, I just tested the one produced here which is a git build and it does have some issues finding the icons? and also the notification window is bigger for some reason, not sure if this is a known issue or an issue that would only affect the appimage for recent builds?

The here link gives me a 404. Could you describe which issues are you getting with the new dunst? I assume only in the master branch?

@Samueru-sama
Copy link
Author

Samueru-sama commented Nov 2, 2024

This PR introduces the generation of an AppImage.
Some context here
The produced appimage uses the static appimage runtime and bundles all the libraries and its own ld-musl.so, so it should work on any linux system from the last 15 years.
Also I'm creating some dummy icon and .desktop entry, it would be nice if dunst gets its official .desktop and icon.

Also while the AppImage that I produce from the last stable build works perfectly, I just tested the one produced here which is a git build and it does have some issues finding the icons? and also the notification window is bigger for some reason, not sure if this is a known issue or an issue that would only affect the appimage for recent builds?

The here link gives me a 404. Could you describe which issues are you getting with the new dunst? I assume only in the master branch?

This PR introduces the generation of an AppImage.
Some context here
The produced appimage uses the static appimage runtime and bundles all the libraries and its own ld-musl.so, so it should work on any linux system from the last 15 years.
Also I'm creating some dummy icon and .desktop entry, it would be nice if dunst gets its official .desktop and icon.

Also while the AppImage that I produce from the last stable build works perfectly, I just tested the one produced here which is a git build and it does have some issues finding the icons? and also the notification window is bigger for some reason, not sure if this is a known issue or an issue that would only affect the appimage for recent builds?

The here link gives me a 404. Could you describe which issues are you getting with the new dunst? I assume only in the master branch?

https://github.com/Samueru-sama/dunst/actions/runs/11643737551

scroll to the end and download the appimage.

And about the issue I ran into, I will open an issue in detail for it with screenshots and all of that later.

UPDATE:

Context for others, the issue was resolved here.

Copy link
Member

@bebehei bebehei left a comment

Choose a reason for hiding this comment

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

Grandpa's review is here! I just read up my e-mails and found #432.

Especially look for the differences in POSIX and bash compatible shells. You can look up the standard: https://pubs.opengroup.org/onlinepubs/9799919799/

The frame based HTML view is totally archaic and cumbersome in the 2020s. But you can look into Shell & Utilites volume. Time invested there is not wasted.

Also the Docker best practices is always a good read for building CI/CD pipelines.

AppImage/make-appimage.sh Show resolved Hide resolved
AppImage/make-appimage.sh Show resolved Hide resolved
AppImage/make-appimage.sh Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
AppImage/make-appimage.sh Show resolved Hide resolved
AppImage/make-appimage.sh Outdated Show resolved Hide resolved
AppImage/make-appimage.sh Show resolved Hide resolved
AppImage/make-appimage.sh Outdated Show resolved Hide resolved
@Samueru-sama
Copy link
Author

@bebehei Are you sure I have bashism in the AppRun? The script passes all checks in shellcheck and I use dash as my /bin/sh.

@bebehei
Copy link
Member

bebehei commented Nov 10, 2024

@Samueru-sama Yes, $() is now in default POSIX. But still I think "$@" is a bashism, because it copies ARGV exactly as the list from ARGV without doing word splitting etc but also not being converted to a concatenated string.

Co-authored-by: Benedikt Heine <[email protected]>
@bebehei
Copy link
Member

bebehei commented Nov 10, 2024

Ok, about "$@", even the POSIX section is totally contradictory:

When the expansion occurs within double-quotes, the behavior is unspecified unless one of the following is true:

  • Field splitting as described in Field Splitting would be performed if the expansion were not within double-quotes (regardless of whether field splitting would have any effect; for example, if IFS is null).

(text bold by me) To me, this makes no sense.

After all, let's just assume, this is no bashism.

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.16%. Comparing base (9d3c91a) to head (c4c6305).
Report is 7 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
+ Coverage   65.11%   65.16%   +0.04%     
==========================================
  Files          50       50              
  Lines        8649     8649              
  Branches     1021     1021              
==========================================
+ Hits         5632     5636       +4     
+ Misses       3017     3013       -4     
Flag Coverage Δ
unittests 65.16% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Samueru-sama
Copy link
Author

Samueru-sama commented Nov 10, 2024

Fixed the AppRun because the new one wasn't working like it should. it is a bit confusing how it works so here is an explanation.

The AppImage runtime sets a few env variables, one of those is the env variable ARGV0 which is just $0, so if you rename or symlink the appimage to be dunstctl for example, you can use that to launch dunstctl or other binaries.

So I transfer that to a new variable called BIN and I unset ARGV0, the reason behind this is because ARGV0 is also used by zsh and can cause issues with zsh users, see here

So the first case statement checks if BIN matches dunst, dunstify or dunstctl and launches them, since dunstctl is a script and not an elf it has to be launched in a different matter without using the dynamic linker.

Then if none of this matches, there is a fallback to check the first argument if it matches dunst, dunstify or dunstctl again, if so we shift and continue as normal.

And if none of this happens the instructions are given and the appimage defaults to running dunst.

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.

4 participants