Re: [aur-general] Package review bitcoin-classic

[Bruno Pagani via aur-general]

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] TU Application: Bruno Pagani

[Maxime Gauduin]

January 10, 2017 10:31 PM, "Bruno Pagani via aur-general" 
 wrote:

> Le 08/01/2017 à 19:19, Maxime Gauduin a écrit :
> 
>> You piqued my interest with bs1770gain, any idea how it fares speed-wise 
>> against the default
>> gstreamer backend? I kept using wine + foobar2000 to compute my replaygain 
>> values even after
>> transitioning to beets because it's infinitely faster than using gstreamer.
> 
> Hi again,
> 
> So, after a little bug fix[0], I ran `time beet replaygain -a` (so Album
> mode, which I expected to be your use case) command on my newly imported
> (with `-A`and into a new library for this test purpose) incoming folder,
> which has the following stats:
> Tracks: 2556
> Total time: 1.1 weeks
> Approximate total size: 102.0 GiB
> Artists: 150
> Albums: 176
> Album artists: 56
> 
> And here is the output of the `time` command:
> beet replaygain -a 2775,24s user 32,68s system 80% cpu 58:00,41 total
> 
> Note this is on a HDD, which I heard spinning a lot during all the
> process. So results on a SSD might differ, I could test that this WE if
> you want, but here we can say roughly 1s per track.
> 
> How does it compare to your workflow?
> 

Thanks for doing the testing, I too did some tests on my server over a sample 
of 373 representative files (flac, m4a, mp3). HDD as well but I only had 15MB/s 
of disk I/O so it's definitely CPU bound. Here are the results:

gstreamer -> roughly 4s per track
bs1770gain -> roughly 3s per track

bs1770gain is definitely faster, but right now I'm using fb2k over the network 
where it takes 0.4s per track. Mind you, my desktop has a much more powerful 
CPU, and fb2k actually uses several threads which afaict beets does not.

I'm quite sure I can divide the time it takes on my server by 4 using threads 
as well which would make beets a viable alternative, I'll look into it when I'm 
more familiar with ayncio. Also found out about the volumedetect filter of 
ffmpeg, but I'd need to document myself more about replaygain and its variants 
to make good use of the output.

> Cheers,
> Bruno
> 
> [0] https://github.com/beetbox/beets/issues/2382

Cheers,
-- 
Maxime

Re: [aur-general] Package review bitcoin-classic

[Bruno Pagani via aur-general]

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=packages/transmission


signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Eli Schwartz via aur-general]

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Bruno Pagani via aur-general]

Le 11/01/2017 à 16:19, Doug Newgard a écrit :

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Bruno Pagani via aur-general]

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Not receiving own emails send to the list

[Bruno Pagani via aur-general]

Le 11/01/2017 à 17:37, Vanush Misha Paturyan via aur-general a écrit :

> On Wed, Jan 11, 2017 at 01:08:07AM +0100, Bruno Pagani via aur-general wrote:
>> Hi there,
>>
>> With my recent email address change followed by my email spree to this
>> list (because of my TU application mostly), I’ve noticed I’m not
>> receiving my own emails, despite “Receive your own posts to the list?”
>> being set to “Yes”.
> I might be wrong here, but I think it is gmail's "feature" not to show
> you your own emails that are coming back via mailing lists. I was
> investigating similar complain by a gmail user that he does not
> receive his own emails via mailing list, and my conclusion was gmail
> silently deletes them.

And it seems you’re right[0]. Thanks for this hint!

This *non-disengageable* “feature” is apparently quite old, but I’m
discovering it only now because this address wasn’t subscribed to much
MLs before…

Screw you Google, at the precise moment rspamd can work with OpenSMTPd
again, you’re off.

Bruno

[0] https://productforums.google.com/forum/#!topic/gmail/fkbKPajx1Dw



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Eli Schwartz via aur-general]

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Tom Zander]

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

Re: [aur-general] Not receiving own emails send to the list

[Vanush Misha Paturyan via aur-general]

On Wed, Jan 11, 2017 at 01:08:07AM +0100, Bruno Pagani via aur-general wrote:
> Hi there,
> 
> With my recent email address change followed by my email spree to this
> list (because of my TU application mostly), I’ve noticed I’m not
> receiving my own emails, despite “Receive your own posts to the list?”
> being set to “Yes”.
>
> I suppose this is linked to SPF/DKIM/DMARC and the fact mailman
> sometimes (based on original sender domains using those techs I guess)
> replaces the From address while putting the original sender in the CC
> field. Maybe when it does so it forgets to send also to this CC address,
> and doesn’t send through the ML because of the “Avoid duplicate copies
> of messages” being triggered by the CC field?
> 
> Can anyone confirm this? I suppose any GMail user can for instance…
> 
> Note, for this email, I’m trying “Avoid duplicate copies of messages” to
> “No” in order to check whether it changes anything, which I kind of
> expect given my current analysis of this issue, but that isn’t a proper
> solution since it will indeed results in duplicate copies.

I might be wrong here, but I think it is gmail's "feature" not to show
you your own emails that are coming back via mailing lists. I was
investigating similar complain by a gmail user that he does not
receive his own emails via mailing list, and my conclusion was gmail
silently deletes them.

Hope this helps

Misha.


-- 
Vanush "Misha" Paturyan
Senior Technical Officer
Room 120
Computer Science Department
Eolas Bulding
Maynooth University
Maynooth

ext: 4539


signature.asc
Description: PGP signature

Re: [aur-general] Package review bitcoin-classic

[Doug Newgard]

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

Re: [aur-general] Package review bitcoin-classic

[Bruno Pagani via aur-general]

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



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] Package review bitcoin-classic

[Tom Zander]

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

Re: [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