[aur-general] Package review bitcoin-classic

[Doug Newgard]

On Wed, 11 Jan 2017 13:22:05 +0100
Tom Zander <tomz at 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!