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

Make PKGBUILD much less flaky and ideompotent #161

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

Conversation

iacore
Copy link

@iacore iacore commented Jun 26, 2022

Now building package for archlinux is easier than before! Just run makepkg -s inside archlinux/ and you are good to go. No more git clean needed.

May break build system.

The newly produced packages are located in archlinux/ beside PKGBUILD, not at git repo root.

Copy link
Member

@marmarek marmarek left a comment

Choose a reason for hiding this comment

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

You correctly anticipated it will break automatic builds: https://gitlab.com/QubesOS/qubes-gui-agent-linux/-/jobs/2642172318

Generally I'm fine with changing it to run makepkg from archlinux dir, but it needs to be done without breaking existing packages. Probably change something in https://github.com/QubesOS/qubes-builder-archlinux/blob/master/Makefile.archlinux#L123-L124 and make the dir conditional on some new variable in Makefile.builder in the component (gui-agent-linux here). Call it ARCH_MAKEPKG_IN_SUBDIR = 1? Any better idea for the name?

@@ -71,9 +88,8 @@ install -D $srcdir/PKGBUILD-z-qubes-session.sh $pkgdir/etc/X11/xinit/xinitrc.d/z
package_qubes-vm-pulseaudio() {

pkgdesc="Pulseaudio support for Qubes VM"
depends=('alsa-lib' 'alsa-utils' 'pulseaudio-alsa' 'pulseaudio<=16.1')
depends=('alsa-lib' 'alsa-utils' 'pulseaudio-alsa' 'pulseaudio')
Copy link
Member

Choose a reason for hiding this comment

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

This should stay (but maybe let it use $pa_ver if that's allowed?) - otherwise pulseaudio update will silently break sound.


build() {

for source in Makefile appvm-scripts gui-common include pulse gui-agent common xf86-input-mfndev xf86-video-dummy xf86-qubes-common window-icon-updater version; do
(ln -s $srcdir/../$source $srcdir/$source)
rm $srcdir/PKGBUILD-z-qubes-session.sh || true
Copy link
Member

Choose a reason for hiding this comment

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

rm -f instead of || true ?

Copy link
Author

@iacore iacore Jun 28, 2022

Choose a reason for hiding this comment

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

It will still fail if the file doesn't exist. makepkg still abort if any command return non-zero.

Copy link
Member

Choose a reason for hiding this comment

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

$ rm -f /no-such-file; echo $?
0

Copy link
Author

@iacore iacore Jun 28, 2022

Choose a reason for hiding this comment

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

I removed clean(). It was for the broken build script before this patch. Now we can use makepkg -c to clean after build.


rm -f pulse/pulsecore
rm -rf pulse/pulsecore
Copy link
Member

Choose a reason for hiding this comment

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

-r sounds risky here - pulse/pulsecore is a symlink. If for some reason it isn't, better fail the build than remove some potentially wanted files...

Copy link
Author

@iacore iacore Jun 28, 2022

Choose a reason for hiding this comment

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

It's not symlink. Check out the git repository. It's a directory.

I made makepkg copy the files first before building, so it should be safe.

pkgver=$(cat $startdir/../version)

clean() {
rm $srcdir/* -r || true
Copy link
Member

Choose a reason for hiding this comment

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

Please options before positional arguments (-r before $srcdir/*).

rm $srcdir/PKGBUILD-z-qubes-session.sh || true
cp $startdir/PKGBUILD-z-qubes-session.sh $srcdir/
for source in Makefile appvm-scripts gui-common include pulse gui-agent common xf86-input-mfndev xf86-video-dummy xf86-qubes-common window-icon-updater version; do
rm $source -r || true
Copy link
Member

Choose a reason for hiding this comment

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

rm -r here too. And avoid || true - if there is some specific error to ignore, prevent it from happening (I guess you meant No such file or directory here -> if [ -e "$source" ]; then rm -r "$source"; fi

md5sums=() #generate with 'makepkg -g'
pa_ver=$((pkg-config --modversion libpulse 2>/dev/null || echo 0.0) | cut -f 1 -d "-")
pa_ver=16.1 #specify this manually please
# If using pkg-config, you must update libpulse first.
Copy link
Member

Choose a reason for hiding this comment

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

Automatic builds (via qubes-builder-archlinux) do install updates before building a package, so I'd prefer to keep it automatic (one place less to change by hand).

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