[aur-general] Package review bitcoin-classic

[Tom Zander]

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