Review request for a PKGBUILD

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Review request for a PKGBUILD

Leonid Bloch-2
Hi,

I'd like to push the following PKGBUILD to AUR. It is useful for those
who want to use HDF5 files with LZ4 compression.

There are two things that bother me about it:
1) The upstream repository contains 4 directories: LZ4, BLOSC, BZIP2,
and docs. Of which only LZ4 is used. I just "git rm -rf" the unneeded
directories, as you can see. But it doesn't seem very elegant.
2) The package will work properly only if the "HDF5_PLUGIN_PATH" env.
variable is set. Putting hdf5_env.sh into /etc/profile.d takes care of
that for any subsequent login. But is there a way to set this variable
in the same shell from which the installation is being performed? And
to unset it on removal? It's not critical at all - no problem to open
a new shell after installation, just for the sake of correctness.

Of course, if there are other problems - I'd like to hear as well.

Thanks!
Leonid.
___

PKGBUILD:
~~~~~~~~~~
# Package maintainer: Leonid B <leonid dot bloch at esrf dot fr>
pkgname=hdf5-lz4-filter-git
pkgver=r106.863db28
pkgrel=1
pkgdesc="LZ4 filter for the HDF5 data format"
arch=('i686' 'x86_64')
url="https://github.com/nexusformat/HDF5-External-Filter-Plugins/tree/master/LZ4"
license=('BSD')
depends=('hdf5' 'lz4')
makedepends=('git')
source=("${pkgname%-git}::git+https://github.com/nexusformat/HDF5-External-Filter-Plugins.git"
        "hdf5_env.sh")
sha1sums=('SKIP' 'c82f6a025138c3f9430ab930a7dd42ad0963918b')

pkgver() {
    cd "${srcdir}/${pkgname%-git}"
    printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse
--short HEAD)"
}

prepare() {
    cd "${srcdir}/${pkgname%-git}"
    git rm -rf BLOSC BZIP2 docs
}

build() {
    cd "${srcdir}/${pkgname%-git}/LZ4"
    ./configure --prefix=/usr --with-hdf5=/usr --with-lz4lib=/usr
    make
}

package() {
    cd "${srcdir}/${pkgname%-git}/LZ4"
    make DESTDIR="${pkgdir}/" install
    install -D -m755 "${srcdir}/hdf5_env.sh"
"${pkgdir}/etc/profile.d/hdf5_env.sh"
    install -D -m644 COPYING "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
}


hdf5_env.sh:
~~~~~~~~~~~
export HDF5_PLUGIN_PATH=/usr/lib
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
On 01/07/2017 11:59 PM, Leonid Bloch wrote:
> Hi,
>
> I'd like to push the following PKGBUILD to AUR. It is useful for those
> who want to use HDF5 files with LZ4 compression.
>
> There are two things that bother me about it:
> 1) The upstream repository contains 4 directories: LZ4, BLOSC, BZIP2,
> and docs. Of which only LZ4 is used. I just "git rm -rf" the unneeded
> directories, as you can see. But it doesn't seem very elegant.

Any especial need to rm it? If it isn't used then you can just ignore
that it is there. :)

Your PKGBUILD syntax looks fine, the only small nit I have is that by
convention the printf uses "r%s.g%s" (see the "VCS package guidelines"
wiki page).

sha1 is weak, but given that the /etc/profile.d dropin is downloaded
with the PKGBUILD it is no worse than using md5sums=('SKIP') for git
repos rather than skipping sha256 instead. :D It remains debatable
whether the average AUR maintainer actually checks sources first anyway...

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

Leonid Bloch-2
Eli, thanks for your review!

Comments inline.

On Sun, Jan 8, 2017 at 7:13 AM, Eli Schwartz via aur-general
<[hidden email]> wrote:

> On 01/07/2017 11:59 PM, Leonid Bloch wrote:
>> Hi,
>>
>> I'd like to push the following PKGBUILD to AUR. It is useful for those
>> who want to use HDF5 files with LZ4 compression.
>>
>> There are two things that bother me about it:
>> 1) The upstream repository contains 4 directories: LZ4, BLOSC, BZIP2,
>> and docs. Of which only LZ4 is used. I just "git rm -rf" the unneeded
>> directories, as you can see. But it doesn't seem very elegant.
>
> Any especial need to rm it? If it isn't used then you can just ignore
> that it is there. :)

Space considerations? Is there a problem with removing them if so?

>
> Your PKGBUILD syntax looks fine, the only small nit I have is that by
> convention the printf uses "r%s.g%s" (see the "VCS package guidelines"
> wiki page).

Maybe I'm missing something, but the wiki says "r%s.%s" if there are no tags.

>
> sha1 is weak, but given that the /etc/profile.d dropin is downloaded
> with the PKGBUILD it is no worse than using md5sums=('SKIP') for git
> repos rather than skipping sha256 instead. :D It remains debatable
> whether the average AUR maintainer actually checks sources first anyway...
>

Agree. Will use sha256, just as a good practice. Just as I look at
others' PKGBUILDs for examples, someone might look at mine. :)

> --
> Eli Schwartz
>
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
On 01/08/2017 12:42 AM, Leonid Bloch wrote:
>> Any especial need to rm it? If it isn't used then you can just ignore
>> that it is there. :)
>
> Space considerations? Is there a problem with removing them if so?

No problem, I just figured if it only exists in the build directory
anyway it might not matter. They will probably remove the build
directory afterwards anyway.

> Maybe I'm missing something, but the wiki says "r%s.%s" if there are no tags.

Hmm, I guess I was missing something actually. But I personally like the
consistency with the "tags exist" version, so I will allow my personal
opinions to override the Wiki on that (at least in my own PKGBUILDs). It
was subjective anyway.

Good catch!

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
In reply to this post by Leonid Bloch-2
Le 08/01/2017 à 05:59, Leonid Bloch a écrit :

> pkgver() {
>     cd "${srcdir}/${pkgname%-git}"
>
> prepare() {
>     cd "${srcdir}/${pkgname%-git}"
>
> build() {
>     cd "${srcdir}/${pkgname%-git}/LZ4"
>
> package() {
>     cd "${srcdir}/${pkgname%-git}/LZ4"
Just a tiny thing, but makepkg always start each function into
"${srcdir}$", so you could remove it from these paths (and thus the
quote altogether).

>     install -D -m755 "${srcdir}/hdf5_env.sh"
> "${pkgdir}/etc/profile.d/hdf5_env.sh"

However, don’t remove it here, because you’re not in the "${srcdir}"
anymore at this point. I’m sure you know, but that’s just in case you
could have went too fast in removing ${srcdir} everywhere (I did when I
learned about makepkg and "${srcdir}"). ;)

Regards,
Bruno


signature.asc (531 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
On Sun, Jan 8, 2017 at 4:49 PM, Bruno Pagani via aur-general
<[hidden email]> wrote:
>>     cd "${srcdir}/${pkgname%-git}/LZ4"
>
> Just a tiny thing, but makepkg always start each function into
> "${srcdir}$", so you could remove it from these paths (and thus the
> quote altogether).

Bash gurus recommend keeping the quotes, as ${pkgname%-git} is also a
parameter, and we generally encourage those be quoted.

cheers!
mar77i
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

Doug Newgard-2
On Sun, 8 Jan 2017 20:04:26 +0100
Martin Kühne via aur-general <[hidden email]> wrote:

> On Sun, Jan 8, 2017 at 4:49 PM, Bruno Pagani via aur-general
> <[hidden email]> wrote:
> >>     cd "${srcdir}/${pkgname%-git}/LZ4"  
> >
> > Just a tiny thing, but makepkg always start each function into
> > "${srcdir}$", so you could remove it from these paths (and thus the
> > quote altogether).  
>
> Bash gurus recommend keeping the quotes, as ${pkgname%-git} is also a
> parameter, and we generally encourage those be quoted.
>
> cheers!
> mar77i

$pkgname is controlled by the PKGBUILD and is guaranteed to to contain spaces.
Quotes wouldn't be needed.
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

Doug Newgard-2
On Sun, 8 Jan 2017 13:08:57 -0600
Doug Newgard <[hidden email]> wrote:

> On Sun, 8 Jan 2017 20:04:26 +0100
> Martin Kühne via aur-general <[hidden email]> wrote:
>
> > On Sun, Jan 8, 2017 at 4:49 PM, Bruno Pagani via aur-general
> > <[hidden email]> wrote:  
> > >>     cd "${srcdir}/${pkgname%-git}/LZ4"    
> > >
> > > Just a tiny thing, but makepkg always start each function into
> > > "${srcdir}$", so you could remove it from these paths (and thus the
> > > quote altogether).    
> >
> > Bash gurus recommend keeping the quotes, as ${pkgname%-git} is also a
> > parameter, and we generally encourage those be quoted.
> >
> > cheers!
> > mar77i  
>
> $pkgname is controlled by the PKGBUILD and is guaranteed to to contain spaces.
> Quotes wouldn't be needed.

Crap, that should be "not to contain", rather than "to to contain"
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
In reply to this post by tur-users mailing list
On 01/08/2017 02:04 PM, Martin Kühne via aur-general wrote:
> Bash gurus recommend keeping the quotes, as ${pkgname%-git} is also a
> parameter, and we generally encourage those be quoted.

Just as many gurus recommend the exact opposite if the *variable* is
guaranteed to not have spaces.

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

Leonid Bloch-2
In reply to this post by tur-users mailing list
Bruno, thanks for the catch! You are right!

But I prefer to keep the quotes: too many times I fell for some
unexplained behavior in Bash the reason for which was forgetting to
quote a variable. I try to quote wherever possible since. :)

Updated PKGBUILD:
~~~~~~~~~~~~~~~~~
# Package maintainer: Leonid B <leonid dot bloch at esrf dot fr>
pkgname=hdf5-lz4-filter-git
pkgver=r106.g863db28
pkgrel=1
pkgdesc="LZ4 filter for the HDF5 data format"
arch=('i686' 'x86_64')
url="https://github.com/nexusformat/HDF5-External-Filter-Plugins/tree/master/LZ4"
license=('BSD')
depends=('hdf5' 'lz4')
makedepends=('git')
source=("${pkgname%-git}::git+https://github.com/nexusformat/HDF5-External-Filter-Plugins.git"
        "hdf5_env.sh")
sha256sums=('SKIP'
            '643d90a15a5105d891adea12806d468aef134f902a38761e864a1085370fb4f9a')

pkgver() {
    cd "${pkgname%-git}"
    printf "r%s.g%s" "$(git rev-list --count HEAD)" "$(git rev-parse
--short HEAD)"
}

prepare() {
    cd "${pkgname%-git}"
    git rm -rf BLOSC BZIP2 docs
}

build() {
    cd "${pkgname%-git}/LZ4"
    ./configure --prefix=/usr --with-hdf5=/usr --with-lz4lib=/usr
    make
}

package() {
    cd "${pkgname%-git}/LZ4"
    make DESTDIR="${pkgdir}/" install
    install -D -m755 "${srcdir}/hdf5_env.sh"
"${pkgdir}/etc/profile.d/hdf5_env.sh"
    install -D -m644 COPYING "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
}

On Sun, Jan 8, 2017 at 5:49 PM, Bruno Pagani via aur-general
<[hidden email]> wrote:

> Le 08/01/2017 à 05:59, Leonid Bloch a écrit :
>
>> pkgver() {
>>     cd "${srcdir}/${pkgname%-git}"
>>
>> prepare() {
>>     cd "${srcdir}/${pkgname%-git}"
>>
>> build() {
>>     cd "${srcdir}/${pkgname%-git}/LZ4"
>>
>> package() {
>>     cd "${srcdir}/${pkgname%-git}/LZ4"
>
> Just a tiny thing, but makepkg always start each function into
> "${srcdir}$", so you could remove it from these paths (and thus the
> quote altogether).
>
>>     install -D -m755 "${srcdir}/hdf5_env.sh"
>> "${pkgdir}/etc/profile.d/hdf5_env.sh"
>
> However, don’t remove it here, because you’re not in the "${srcdir}"
> anymore at this point. I’m sure you know, but that’s just in case you
> could have went too fast in removing ${srcdir} everywhere (I did when I
> learned about makepkg and "${srcdir}"). ;)
>
> Regards,
> Bruno
>
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
Le 08/01/2017 à 22:13, Leonid Bloch a écrit :

> Bruno, thanks for the catch! You are right!
>
> But I prefer to keep the quotes: too many times I fell for some
> unexplained behavior in Bash the reason for which was forgetting to
> quote a variable. I try to quote wherever possible since. :)

Well that’s a good precaution generally, and I know what I’m talking
about since my involvement in Bumblebee (and FLOSS more generally)
started partially because someone didn’t took it[0], but here see Eli
and Doug comments, this is really unnecessary. ;)

Cheers,
Bruno

[0]
https://github.com/MrMEEE/bumblebee-Old-and-abbandoned/commit/a047be85247755cdbe0acce6f1dafc8beb84f2ac


signature.asc (531 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
On 01/08/2017 04:35 PM, Bruno Pagani via aur-general wrote:
> started partially because someone didn’t took it[0], but here see Eli
> and Doug comments, this is really unnecessary. ;)

Would this be a bad time to say that my personal style of choice is to
over-quote? :p

All I said was "appeal to authority" == fail because you can get gurus
to say whatever you want. It's a very subjective bikeshed.

As long as you *do* use quotes in circumstances where you *can* get
unpredictable user input containing spaces (or nothing of course!) it
really doesn't matter what you do the rest of the time.
It is a style preference.

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
Le 08/01/2017 à 22:55, Eli Schwartz via aur-general a écrit :

> On 01/08/2017 04:35 PM, Bruno Pagani via aur-general wrote:
>> started partially because someone didn’t took it[0], but here see Eli
>> and Doug comments, this is really unnecessary. ;)
> Would this be a bad time to say that my personal style of choice is to
> over-quote? :p

Well, in that case, I’ll call only to Doug. ;p

> All I said was "appeal to authority" == fail because you can get gurus
> to say whatever you want. It's a very subjective bikeshed.
>
> As long as you *do* use quotes in circumstances where you *can* get
> unpredictable user input containing spaces (or nothing of course!) it
> really doesn't matter what you do the rest of the time.
> It is a style preference.

You’re absolutely right. :) My own preference goes to less chars, and
thus making sure I know what I’m doing. ;)

But I’m OK with people over-quoting, like you said it’s under-quoting
which is an issue. I was just applying a second layer of “there is
nothing to be feared here in the absence of quotes”.

Bruno


signature.asc (531 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
Thanks for my sunday night's identiy crisis, I can no longer tell
whether I was implying myself as an authority there or not. Or whether
I intended to imply somesuch.

cheers!
mar77i
Reply | Threaded
Open this post in threaded view
|

Re: Review request for a PKGBUILD

tur-users mailing list
On 01/08/2017 05:42 PM, Martin Kühne via aur-general wrote:
> Thanks for my sunday night's identiy crisis, I can no longer tell
> whether I was implying myself as an authority there or not. Or whether
> I intended to imply somesuch.

Happy to be of service. :p

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment