[aur-general] Package review bitcoin-classic
Hi, I created some AUR packages after various people requested it (I'm the upstream release manager) and since I'm rather new to packaging in Arch, I would love to have some feedback. Arch packaging is consederably easier than debian, I might add ;) There are 4, almost identical, versions. https://aur.archlinux.org/packages/bitcoin-classic/ https://aur.archlinux.org/packages/bitcoin-classic-daemon/ https://aur.archlinux.org/packages/bitcoin-classic-git/ https://aur.archlinux.org/packages/bitcoind-classic-git/ I inherited some of those, hence the slight difference in naming. Not sure if renaming is possible and if the bitcoind- one may be more easy to find under a different name. Any feedback welcome, and naturally I'd love it if those packages would be able to reach the community repo, but maybe they need a little more time to mature, I'm not sure. Thanks! -- Tom Zander Blog: https://zander.github.io Vlog: https://vimeo.com/channels/tomscryptochannel
Hi Tom, Le 07/01/2017 à 16:41, Tom Zander a écrit :
Hi,
I created some AUR packages after various people requested it (I'm the upstream release manager) and since I'm rather new to packaging in Arch, I would love to have some feedback. Arch packaging is consederably easier than debian, I might add ;)
Definitively agree. I came to Arch because of the philosophy, I stay here because of PKGBUILD among other things. ;)
There are 4, almost identical, versions.
https://aur.archlinux.org/packages/bitcoin-classic/ https://aur.archlinux.org/packages/bitcoin-classic-daemon/ https://aur.archlinux.org/packages/bitcoin-classic-git/ https://aur.archlinux.org/packages/bitcoind-classic-git/
I inherited some of those, hence the slight difference in naming. Not sure if renaming is possible and if the bitcoind- one may be more easy to find under a different name.
I’ll eventually make a review a bit latter (need to fix things following review of my own packages), but regarding the name, yes it can be changed: 1. Change the pkgname in the PKGBUILD. 2. Get current origin remote URL using `git remote -v` in your local package git clone. 3. Change it to the new name using `git remote set-url origin <newURL>`. 4. Push changes. 5. File a merge request on the old package toward the new one. This way, you will preserve git history, comments and votes (and even maybe notifications for suscribed users, but I’m not sure about that one — if not, this could be a nice feature to have in AUR). I’m not sure whether such move are very common, if so maybe the AUR web could propose such a feature (or maybe it does exist and I missed it…). Regarding naming for daemon between `d` suffix or `-daemon`, I have no opinion and either most people here don’t have one or agree to the only answer you got before[0], but it should indeed be consistent between VCS and non-VCS package of the same software. ;)
Any feedback welcome, and naturally I'd love it if those packages would be able to reach the community repo, but maybe they need a little more time to mature, I'm not sure.
Thanks!
I might come back to you once more when I’ll had the time to review your packages, but I think some (more experienced) users will provide some feedback in between and that might not be necessary anymore. ;) Cheers, Bruno [0] https://lists.archlinux.org/pipermail/aur-general/2016-December/033071.html
On Saturday, 7 January 2017 18:53:46 CET Bruno Pagani via aur-general wrote:
There are 4, almost identical, versions.
https://aur.archlinux.org/packages/bitcoin-classic/ https://aur.archlinux.org/packages/bitcoin-classic-daemon/ https://aur.archlinux.org/packages/bitcoin-classic-git/ https://aur.archlinux.org/packages/bitcoind-classic-git/
I inherited some of those, hence the slight difference in naming. Not sure if renaming is possible and if the bitcoind- one may be more easy to find under a different name.
I’ll eventually make a review a bit latter (need to fix things following review of my own packages), but regarding the name, yes it can be changed:
[snip] Thanks, I went through the motions. Looks good. Thanks for the detailed help!
Regarding naming for daemon between `d` suffix or `-daemon`, I have no opinion and either most people here don’t have one or agree to the only answer you got before[0], but it should indeed be consistent between VCS and non-VCS package of the same software. ;)
I went for the “foo-daemon-git” solution, makes sense to me.
Any feedback welcome, and naturally I'd love it if those packages would be able to reach the community repo, but maybe they need a little more time to mature, I'm not sure.
I might come back to you once more when I’ll had the time to review your packages, but I think some (more experienced) users will provide some feedback in between and that might not be necessary anymore. ;)
Thanks! And good luck with your TU application! :) -- Tom Zander Blog: https://zander.github.io Vlog: https://vimeo.com/channels/tomscryptochannel
On Sat, 07 Jan 2017 16:41:42 +0100 Tom Zander <tomz@freedommail.ch> wrote:
Hi,
I created some AUR packages after various people requested it (I'm the upstream release manager) and since I'm rather new to packaging in Arch, I would love to have some feedback. Arch packaging is consederably easier than debian, I might add ;)
There are 4, almost identical, versions.
https://aur.archlinux.org/packages/bitcoin-classic/ https://aur.archlinux.org/packages/bitcoin-classic-daemon/ https://aur.archlinux.org/packages/bitcoin-classic-git/ https://aur.archlinux.org/packages/bitcoind-classic-git/
I inherited some of those, hence the slight difference in naming. Not sure if renaming is possible and if the bitcoind- one may be more easy to find under a different name.
Any feedback welcome, and naturally I'd love it if those packages would be able to reach the community repo, but maybe they need a little more time to mature, I'm not sure.
Thanks!
Alright, I've looked at the first one. There's a whole lot of simplification you could do, and a few actual problems. Problems: You need to rename the source file. It's just "v1.2.0.tar.gz" which is very generic and could conflict with other packages. Hardcoding the version here also makes no sense when you could just use $pkgver and only have to update it in one place when you update. The install file does WAY, WAY more than it should. The only thing I see there that should be happening is creating the user and group, everything else should be removed. make -j$(nproc) is bad. The user sets makeflags in makepkg.conf, you should not be overriding that unless it's necessary. Simplifications: Functions are guaranteed to start in $srcdir, you don't need to include that in the cd commands. All of the "msg2" lines are useless. In the package function, you cd to $srcdir/bitcoinclassic-$pkgver, but then you still include that in every install command. It's redundant and unnecessary. The cp then desktop-file-install for the .desktop file is really unnecessary when you could just install it like everything else. Some of the install commands could be combined. If you're not renaming the file, you can just specify a target dir with -t and install all files to that dir at once. For renaming, just file a merge request on the right hand side of the page to merge one package into the other. Doug
On Saturday, 7 January 2017 14:29:58 CET Doug Newgard wrote:
Alright, I've looked at the first one. There's a whole lot of simplification you could do, and a few actual problems.
Problems:
You need to rename the source file. It's just "v1.2.0.tar.gz" which is very generic and could conflict with other packages.
This is what github creates when you push a tag, I have no choice in the matter. Why would there be a conflict? This is only used for makepkg and any package that is created is always compiled in its own dir. The PKGBUILD file doesn’t have a project name in front of it either, ensuring its all done in its own dir... What am I missing?
Hardcoding the version here also makes no sense when you could just use $pkgver and only have to update it in one place when you update.
Hmm? I did exactlyt that. The pkver is set once and used everywhere... Maybe you looked at a ‘-git’ package?
The install file does WAY, WAY more than it should. The only thing I see there that should be happening is creating the user and group, everything else should be removed.
Maybe you are assuming that a ‘make install’ will be able to do everything we need. Unfortunately thats not the case... And I don’t want to change the upstream automake files. So individual install lines are needed.
make -j$(nproc) is bad. The user sets makeflags in makepkg.conf, you should not be overriding that unless it's necessary.
ACK.
Simplifications:
Functions are guaranteed to start in $srcdir, you don't need to include that in the cd commands.
ACK
All of the "msg2" lines are useless.
ACK
In the package function, you cd to $srcdir/bitcoinclassic-$pkgver, but then you still include that in every install command. It's redundant and unnecessary.
ACK
The cp then desktop-file-install for the .desktop file is really unnecessary when you could just install it like everything else.
ACK
Some of the install commands could be combined. If you're not renaming the file, you can just specify a target dir with -t and install all files to that dir at once.
ACK
For renaming, just file a merge request on the right hand side of the page to merge one package into the other.
will do :)
Doug
Thanks Doug! -- Tom Zander Blog: https://zander.github.io Vlog: https://vimeo.com/channels/tomscryptochannel
Le 11/01/2017 à 13:22, Tom Zander a écrit :
On Saturday, 7 January 2017 14:29:58 CET Doug Newgard wrote:
Alright, I've looked at the first one. There's a whole lot of simplification you could do, and a few actual problems.
Problems:
You need to rename the source file. It's just "v1.2.0.tar.gz" which is very generic and could conflict with other packages. This is what github creates when you push a tag, I have no choice in the matter. Why would there be a conflict? This is only used for makepkg and any package that is created is always compiled in its own dir. The PKGBUILD file doesn’t have a project name in front of it either, ensuring its all done in its own dir... What am I missing?
Use of $SRCDEST by some tool like pacaur, and how main repos works: all sources for all packages are stored together. So any other packages with "v1.2.0.tar.gz" source will be in conflict. To fix this, your source array should do something like that: source=(${pkgname}-${pkgver}.tar.gz::"<_srcurl>.tar.gz")
Hardcoding the version here also makes no sense when you could just use $pkgver and only have to update it in one place when you update. Hmm? I did exactlyt that. The pkver is set once and used everywhere... Maybe you looked at a ‘-git’ package?
I’ll check what Doug meant in my forthcoming review.
The install file does WAY, WAY more than it should. The only thing I see there that should be happening is creating the user and group, everything else should be removed. Maybe you are assuming that a ‘make install’ will be able to do everything we need. Unfortunately thats not the case... And I don’t want to change the upstream automake files. So individual install lines are needed.
I didn’t do my review yet, but if they are things to be done to “fix make install”, they probably should be in the package function, not the install file. I’ll come back to you later today regarding this, once I’ll had the time to review your packages (currently at work). Keep tuned, Bruno
On Wed, 11 Jan 2017 13:22:05 +0100 Tom Zander <tomz@freedommail.ch> wrote:
On Saturday, 7 January 2017 14:29:58 CET Doug Newgard wrote:
Alright, I've looked at the first one. There's a whole lot of simplification you could do, and a few actual problems.
Problems:
You need to rename the source file. It's just "v1.2.0.tar.gz" which is very generic and could conflict with other packages.
This is what github creates when you push a tag, I have no choice in the matter. Why would there be a conflict? This is only used for makepkg and any package that is created is always compiled in its own dir. The PKGBUILD file doesn’t have a project name in front of it either, ensuring its all done in its own dir... What am I missing?
Hardcoding the version here also makes no sense when you could just use $pkgver and only have to update it in one place when you update.
Hmm? I did exactlyt that. The pkver is set once and used everywhere... Maybe you looked at a ‘-git’ package?
source=(${pkgname}-${pkgver}.tar.gz::"https://github.com/bitcoinclassic/bitcoinclassic/archive/v1.2.0.tar.gz" Sure looks hardcoded to me.
The install file does WAY, WAY more than it should. The only thing I see there that should be happening is creating the user and group, everything else should be removed.
Maybe you are assuming that a ‘make install’ will be able to do everything we need. Unfortunately thats not the case... And I don’t want to change the upstream automake files. So individual install lines are needed.
I'm not talking about the package function, I'm talking about the .install file with the post_{install,upgrade,remove} scriptlets. Specifically: Disabling COW should be left up to the sysadmin. chown should be done in the PKGBUILD if at all possible Users should not be deleted; this is a security issue if the UID gets reused. Your big fancy message shouldn't be there. This is normal stuff. The whole "/etc/bitcoin/bitcoin.conf.dist" thing shouldn't happen. Just install to /etc/bitcoin/bitcoin.conf in the package function.
make -j$(nproc) is bad. The user sets makeflags in makepkg.conf, you should not be overriding that unless it's necessary.
ACK.
Simplifications:
Functions are guaranteed to start in $srcdir, you don't need to include that in the cd commands.
ACK
All of the "msg2" lines are useless.
ACK
In the package function, you cd to $srcdir/bitcoinclassic-$pkgver, but then you still include that in every install command. It's redundant and unnecessary.
ACK
The cp then desktop-file-install for the .desktop file is really unnecessary when you could just install it like everything else.
ACK
Some of the install commands could be combined. If you're not renaming the file, you can just specify a target dir with -t and install all files to that dir at once.
ACK
For renaming, just file a merge request on the right hand side of the page to merge one package into the other.
will do :)
Doug
Thanks Doug!
On Wednesday, 11 January 2017 09:19:45 CET Doug Newgard wrote:
I'm not talking about the package function, I'm talking about the .install file with the post_{install,upgrade,remove} scriptlets. Specifically:
My apologies, I must have misread :) I have to ask a bit more since your pointers are great, but I’m not sure how I can actually accomplish them:
Disabling COW should be left up to the sysadmin.
How would we do that?
chown should be done in the PKGBUILD if at all possible
Can you explain how? AFAICT the user doesn’t exist yet at that point.
Users should not be deleted; this is a security issue if the UID gets reused. Your big fancy message shouldn't be there. This is normal stuff.
The whole "/etc/bitcoin/bitcoin.conf.dist" thing shouldn't happen. Just install to /etc/bitcoin/bitcoin.conf in the package function.
I have not found out how pacman treats config files on upgrades, removes and reinstalls. The point here is that the config file should not be overwritten by the package version on upgrade, it should not be deleted when the package is deleted (and may be reinstalled just minutes later) etc. Only when its 100% identical to the one in the package can it safely be deleted / upgraded. Maybe I didn’t look good enough, but I could not find out how to do the above. So I went for the safest solution. -- Tom Zander Blog: https://zander.github.io Vlog: https://vimeo.com/channels/tomscryptochannel
On 01/11/2017 12:01 PM, Tom Zander wrote:
I have not found out how pacman treats config files on upgrades, removes and reinstalls. The point here is that the config file should not be overwritten by the package version on upgrade, it should not be deleted when the package is deleted (and may be reinstalled just minutes later) etc. Only when its 100% identical to the one in the package can it safely be deleted / upgraded.
Maybe I didn’t look good enough, but I could not find out how to do the above. So I went for the safest solution.
man -P 'less -p backup' PKGBUILD -- Eli Schwartz
Le 11/01/2017 à 18:01, Tom Zander a écrit :
On Wednesday, 11 January 2017 09:19:45 CET Doug Newgard wrote:
I'm not talking about the package function, I'm talking about the .install file with the post_{install,upgrade,remove} scriptlets. Specifically: My apologies, I must have misread :)
I have to ask a bit more since your pointers are great, but I’m not sure how I can actually accomplish them:
I’ll answer that for Doug. Note that this is my proposition based on systemd tools, there might be other solutions.
Disabling COW should be left up to the sysadmin. How would we do that?
Just don’t do it, eventually just print a message advising the user to do so.
chown should be done in the PKGBUILD if at all possible Can you explain how? AFAICT the user doesn’t exist yet at that point.
You could use systemd-tmpfiles[0] and its Z mode[1]. This tool would also help you managing the other directories creation and the like. Please tell if you need help setting up this.
Users should not be deleted; this is a security issue if the UID gets reused. Your big fancy message shouldn't be there. This is normal stuff.
After a quick glance at your install file, I think it should be as simple and short as this (+eventual notice for CoW): post_install() { systemd-sysusers bitcoin-classic.conf systemd-tmpfiles --create bitcoin-classic.conf } Of course, all of the “hard” work is to be done in new files bitcoin.tmpfiles and bitcoin.sysusers (for the systemd-sysusers[2] feature, related format[3]), that should be installed like this in package(): install -Dm644 "${srcdir}"/${pkgname}.tmpfiles "${pkgdir}"/usr/lib/tmpfiles.d/${pkgname}.conf install -Dm644 "${srcdir}"/${pkgname}.sysusers "${pkgdir}"/usr/lib/sysusers.d/${pkgname}.conf I can help you writing those files if you need.
The whole "/etc/bitcoin/bitcoin.conf.dist" thing shouldn't happen. Just install to /etc/bitcoin/bitcoin.conf in the package function. I have not found out how pacman treats config files on upgrades, removes and reinstalls. The point here is that the config file should not be overwritten by the package version on upgrade, it should not be deleted when the package is deleted (and may be reinstalled just minutes later) etc. Only when its 100% identical to the one in the package can it safely be deleted / upgraded.
Maybe I didn’t look good enough, but I could not find out how to do the above. So I went for the safest solution.
See Eli answer. :) You could guess that for such a widespread need, there is something to handle it. ;) Further reviewing incoming, Bruno [0] https://www.freedesktop.org/software/systemd/man/systemd-tmpfiles.html [1] https://www.freedesktop.org/software/systemd/man/tmpfiles.d.html [2] https://www.freedesktop.org/software/systemd/man/systemd-sysusers.html [3] https://www.freedesktop.org/software/systemd/man/sysusers.d.html
On 01/11/2017 01:15 PM, Bruno Pagani via aur-general wrote:
After a quick glance at your install file, I think it should be as simple and short as this (+eventual notice for CoW): post_install() { systemd-sysusers bitcoin-classic.conf systemd-tmpfiles --create bitcoin-classic.conf }
The latest version of systemd has hooks for both of those. -- Eli Schwartz
Le 11/01/2017 à 19:43, Eli Schwartz via aur-general a écrit :
On 01/11/2017 01:15 PM, Bruno Pagani via aur-general wrote:
After a quick glance at your install file, I think it should be as simple and short as this (+eventual notice for CoW): post_install() { systemd-sysusers bitcoin-classic.conf systemd-tmpfiles --create bitcoin-classic.conf } The latest version of systemd has hooks for both of those.
Thanks for the hint, awesome! That’s even more .install files removed. :) Cheers, Bruno
Le 11/01/2017 à 16:19, Doug Newgard a écrit :
On Wed, 11 Jan 2017 13:22:05 +0100 Tom Zander <tomz@freedommail.ch> wrote:
On Saturday, 7 January 2017 14:29:58 CET Doug Newgard wrote:
Hardcoding the version here also makes no sense when you could just use $pkgver and only have to update it in one place when you update. Hmm? I did exactlyt that. The pkver is set once and used everywhere... Maybe you looked at a ‘-git’ package? source=(${pkgname}-${pkgver}.tar.gz::"https://github.com/bitcoinclassic/bitcoinclassic/archive/v1.2.0.tar.gz"
Sure looks hardcoded to me.
@Tom: Doug message here is that this “1.2.0” string should be ${pkgver}.
Le 07/01/2017 à 16:41, Tom Zander a écrit :
Hi,
I created some AUR packages after various people requested it (I'm the upstream release manager) and since I'm rather new to packaging in Arch, I would love to have some feedback. Arch packaging is consederably easier than debian, I might add ;)
There are 4, almost identical, versions.
https://aur.archlinux.org/packages/bitcoin-classic/ https://aur.archlinux.org/packages/bitcoin-classic-daemon/ https://aur.archlinux.org/packages/bitcoin-classic-git/ https://aur.archlinux.org/packages/bitcoind-classic-git/
I inherited some of those, hence the slight difference in naming. Not sure if renaming is possible and if the bitcoind- one may be more easy to find under a different name.
Any feedback welcome, and naturally I'd love it if those packages would be able to reach the community repo, but maybe they need a little more time to mature, I'm not sure.
Thanks!
So, some days later… Here is my review, which is to be added to Doug comments. Some (most?) are styles choice, so you’re free to follow them or not. ;) Given the high level of similarity of your 4 packages, all comments apply to most of them. That’s also a hint for something bigger: you could probably setup a split PKGBUILD, and have bitcoin-classic to only pack the GUI and depends on bitcoin-daemon-classic for the rest (and same with -git variants). See transmission[0] for example, and don’t hesitate to ask for help if you want to go that way. ;) – Please be consistent in your use of vars: either ${var} or $var, but don’t mixed them. I think the only instance is ${pkgname}-${pkgver}.tar.gz in source array because your borrowed it from my comment. ;) – I think you could declare a _pkgname=bitcoinclassic var and use it in several places. I like my package to make use of var has much has possible to make any change easy: |url="https://$_pkgname.com" ||source=(${pkgname}-${pkgver}.tar.gz::"https://github.com/$_pkgname/$_pkgname/archive/v$pkgver.tar.gz" ||cd "$_pkgname-$pkgver" And so on… | – In the same idea, you could declare a _pkgorig=bitcoin (wasn’t inspired at var name) and use it almost elsewhere (provides/conflicts,package() function). – It’s advised to split ./configure option on multiple lines for increased {read,edit}ability. Example (bitcoin-classic): |./configure \ --prefix=/usr \ --with-incompatible-bdb \ --with-gui=qt5 \ --enable-hardening \ --enable-reduce-exports \ --disable-gui-tests \ --disable-maintainer-mode| – In your -git packages, |cd "$srcdir/bitcoinclassic" | should be: cd "bitcoinclassic" That’d be all for this first pass, I can have another look if you want once you’ll have fixed Doug concerns and implemented the changes you want among the above ones. ;) Cheers, Bruno [0] https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packag...
participants (4)
-
Bruno Pagani
-
Doug Newgard
-
Eli Schwartz
-
Tom Zander