Critique my pkgbuild

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

Critique my pkgbuild

Ethan Rakoff
I have submitted a package called threemawebqt to the aur (mostly for
me, and some friends who use arch). It is a VERY simple thin client for
a webapp using Qt. This is my first pkgbuild from scratch (and my first
time working with Qt) so even though it is a super simple one, I would
like some others to look at it and let me know what I'm doing wrong. Thanks!

Here is the pkgbuild for easy reading:

# Maintainer: Ethan Rakoff <[hidden email]>

pkgname=threemawebqt
pkgver=0.1
pkgrel=1
pkgdesc="Thin client for Threema Web, the web client for Threema, an E2E
encrypted messaging app."
arch=('i686' 'x86_64')
url="https://github.com/ethanrakoff/${pkgname}"
license=('MIT')
depends=('qt5-base' 'qt5-webengine')
makedepends=('make')
source=("git+${url}")
md5sums=('SKIP')

build() {
   cd "${pkgname}/src"

   qmake
   make
}

package() {
   cd "${srcdir}/${pkgname}/src"
   make INSTALL_ROOT="${pkgdir}" install

   install -Dm644 icon.png "${pkgdir}/usr/share/icons/${pkgname}/icon.png"
   install -Dm644 ../threemawebqt.desktop
"${pkgdir}/usr/share/applications/threemawebqt.desktop"
   install -Dm644 ../LICENSE
"${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
}


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

Re: Critique my pkgbuild

tur-users mailing list
On Wed, Oct 10, 2018 at 01:34:25PM +0000, Ethan Rakoff wrote:
> # Maintainer: Ethan Rakoff <[hidden email]>
>
> pkgname=threemawebqt

Needs to have a -git suffix as it builds from a git source and is thus a VCS
package.

> pkgver=0.1

You need a pkgver() function as this is an VCS package. Since you don't have
tags yet you should count the number of revisions.

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

https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git

> pkgrel=1
> pkgdesc="Thin client for Threema Web, the web client for Threema, an E2E encrypted messaging app."
> arch=('i686' 'x86_64')
> url="https://github.com/ethanrakoff/${pkgname}"
> license=('MIT')
> depends=('qt5-base' 'qt5-webengine')
> makedepends=('make')

`make` is present in `base-devel` thus shouldn't be a listed dependency. Unsure
if its however worth listing it as it's the only needed build-time dependency?

> source=("git+${url}")

I personally dislike the need to use variables just because they exist.

This reads much better:
    source=("git+https://github.com/ethanrakoff/threemawebqt")

> md5sums=('SKIP')
>
> build() {
>   cd "${pkgname}/src"
>
>   qmake
>   make
> }
>
> package() {
>   cd "${srcdir}/${pkgname}/src"
You omitted `$srcdir` from `build()` but added it here. This is mostly a style
thing, but I'd just omit it all together.

>   make INSTALL_ROOT="${pkgdir}" install
>
>   install -Dm644 icon.png "${pkgdir}/usr/share/icons/${pkgname}/icon.png"
>   install -Dm644 ../threemawebqt.desktop "${pkgdir}/usr/share/applications/threemawebqt.desktop"
>   install -Dm644 ../LICENSE "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
> }
>




--
Morten Linderud
PGP: 9C02FF419FECBE16

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

Re: Critique my pkgbuild

Tinu Weber
On Wed, Oct 10, 2018 at 15:48:31 +0200, Morten Linderud via aur-general wrote:
> > makedepends=('make')
>
> `make` is present in `base-devel` thus shouldn't be a listed dependency. Unsure
> if its however worth listing it as it's the only needed build-time dependency?

As it is a git package (and git is not in base-devel), I would argue
that at least 'git' should go in there.

Best,
Tinu

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

Re: Critique my pkgbuild

tur-users mailing list
On 10/10/18 9:52 AM, Tinu Weber wrote:
> On Wed, Oct 10, 2018 at 15:48:31 +0200, Morten Linderud via aur-general wrote:
>>> makedepends=('make')
>>
>> `make` is present in `base-devel` thus shouldn't be a listed dependency. Unsure
>> if its however worth listing it as it's the only needed build-time dependency?
>
> As it is a git package (and git is not in base-devel), I would argue
> that at least 'git' should go in there.

In fact, the current PKGBUILD has git as a makedepends because you need
it in order to clone the source.

Programs used for downloading the source must be specified in
makedepends unless it is curl since curl is a dependency of the pacman
package.

--
Eli Schwartz
Bug Wrangler and Trusted User


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

Re: Critique my pkgbuild

tur-users mailing list
In reply to this post by Tinu Weber
On Wed, Oct 10, 2018 at 03:52:49PM +0200, Tinu Weber wrote:
> On Wed, Oct 10, 2018 at 15:48:31 +0200, Morten Linderud via aur-general wrote:
> > > makedepends=('make')
> >
> > `make` is present in `base-devel` thus shouldn't be a listed dependency. Unsure
> > if its however worth listing it as it's the only needed build-time dependency?
>
> As it is a git package (and git is not in base-devel), I would argue
> that at least 'git' should go in there.

Woopsie. That completely escaped me :D


--
Morten Linderud
PGP: 9C02FF419FECBE16

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

Re: Critique my pkgbuild

tur-users mailing list
In reply to this post by Ethan Rakoff
On 10/10/18 9:34 AM, Ethan Rakoff wrote:

> I have submitted a package called threemawebqt to the aur (mostly for
> me, and some friends who use arch). It is a VERY simple thin client for
> a webapp using Qt. This is my first pkgbuild from scratch (and my first
> time working with Qt) so even though it is a super simple one, I would
> like some others to look at it and let me know what I'm doing wrong.
> Thanks!
>
> Here is the pkgbuild for easy reading:
>
> # Maintainer: Ethan Rakoff <[hidden email]>
>
> pkgname=threemawebqt
> pkgver=0.1
> pkgrel=1
> pkgdesc="Thin client for Threema Web, the web client for Threema, an E2E
> encrypted messaging app."
That's a rather lengthy pkgdesc, what about:
"thin client for the Threema Web E2E encrypted messaging app"

> arch=('i686' 'x86_64')
> url="https://github.com/ethanrakoff/${pkgname}"
> license=('MIT')
> depends=('qt5-base' 'qt5-webengine')
> makedepends=('make')

You specified "make" as a dependency, which is already in base-devel,
but not "git", which you need to download sources.

Protip: you can catch issues like this (where your
coincidentally-installed programs are actually needed to build) by
checking that your package still builds in a clean chroot using the
mkarchroot and makechrootpkg commands from [extra]/devtools. Or use the
extra-x86_64-build convenience wrapper.

> source=("git+${url}")

I like it when people use variables just because they exist, so keep up
the good work!

Also this underlines the association between the url and the source
download.

> md5sums=('SKIP')
>
> build() {
>   cd "${pkgname}/src"
>
>   qmake
>   make
> }
>
> package() {
>   cd "${srcdir}/${pkgname}/src"
>   make INSTALL_ROOT="${pkgdir}" install
>
>   install -Dm644 icon.png "${pkgdir}/usr/share/icons/${pkgname}/icon.png"
>   install -Dm644 ../threemawebqt.desktop
> "${pkgdir}/usr/share/applications/threemawebqt.desktop"
>   install -Dm644 ../LICENSE
> "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
> }
Since you're the upstream developer, you have the opportunity to fix
your qmake configuration.

It doesn't respect any sort of --prefix option, but hardcodes
/usr/local/bin/ as the install location. You can have this determined at
qmake time using e.g.
https://stackoverflow.com/questions/7106442/qt-project-files-and-prefix-variable/7106823#7106823

It should not require one to install the desktop file and icon by hand,
but add these as additional "thing.files".

The license file is not required to run, and distributions often have
different ideas about where to install it for distribution packages...
so you don't need to install it too.

...

Correspondingly, the desktop file should not hardcode the Exec path, but
rely on it being in the system $PATH. Likewise, the Icon should not be
/usr/share/icons/threemawebqt/icon.png but instead "threemawebqt", and
you should install icon.png as "threemawebqt.png" to a directory that
obeys the XDG icon theme specification:
https://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#directory_layout

e.g. /usr/share/pixmaps/ or /usr/share/icons/hicolor/128x128/apps/

...

Also your *_DIR setup uses the parent directory but assumes that it is
run from the src/ folder, look what happens if you try to run

cd /tmp
git clone https://github.com/ethanrakoff/threemawebqt /tmp/threemawebqt
cd /tmp/threemawebqt
qmake src/threemawebqt.pro
make
ls /tmp/build

I'd suggest having your .pro file in the repository root, and adding
SOURCES as src/main.cpp then building in build rather than ../build

--
Eli Schwartz
Bug Wrangler and Trusted User


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

Re: Critique my pkgbuild

Ethan Rakoff
Hello again,

It took me a while to have some time, but I believe I have fixed all of the issues that were pointed out to me, with some help from Scimmia and eschwartz. I think the package is in much more presentable shape, I have learned plenty, and I fixed a lot of naive mistakes. Thanks everyone for the help.

I would very much appreciate it if you would all take the time to read the latest PKGBUILD (from the git version of the package) pasted below and see if there is anything I missed. Thanks!

# Maintainer: Ethan Rakoff <[hidden email]>

pkgname=threemawebqt-git
pkgver=r2.079de78
pkgrel=1
pkgdesc="Thin client for Threema Web, an E2E encrypted messaging app."
arch=('i686' 'x86_64')
url="https://github.com/ethanrakoff/${pkgname%-*}"
license=('MIT')
depends=('qt5-base' 'qt5-webengine')
makedepends=('git')
source=("git+${url}")
md5sums=('SKIP')

pkgver() {
  cd "${pkgname%-*}"

  printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse --short HEAD)"

}

build() {
  cd "${pkgname%-*}"

  qmake "PREFIX=/usr"
  make
}

package() {
  cd "${pkgname%-*}"

  make INSTALL_ROOT="${pkgdir}" install


  install -Dm644 LICENSE "${pkgdir}/usr/share/licenses/${pkgname%-*}/LICENSE"
}


----------------
Ethan Rakoff

Sent with ProtonMail Secure Email.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, October 10, 2018 2:32 PM, Eli Schwartz via aur-general <[hidden email]> wrote:

> On 10/10/18 9:34 AM, Ethan Rakoff wrote:
>

> > I have submitted a package called threemawebqt to the aur (mostly for
> > me, and some friends who use arch). It is a VERY simple thin client for
> > a webapp using Qt. This is my first pkgbuild from scratch (and my first
> > time working with Qt) so even though it is a super simple one, I would
> > like some others to look at it and let me know what I'm doing wrong.
> > Thanks!
> > Here is the pkgbuild for easy reading:
> >

> > Maintainer: Ethan Rakoff [hidden email]
> >

> > ===============================================
> >

> > pkgname=threemawebqt
> > pkgver=0.1
> > pkgrel=1
> > pkgdesc="Thin client for Threema Web, the web client for Threema, an E2E
> > encrypted messaging app."
>

> That's a rather lengthy pkgdesc, what about:
> "thin client for the Threema Web E2E encrypted messaging app"
>

> > arch=('i686' 'x86_64')
> > url="https://github.com/ethanrakoff/${pkgname}"
> > license=('MIT')
> > depends=('qt5-base' 'qt5-webengine')
> > makedepends=('make')
>

> You specified "make" as a dependency, which is already in base-devel,
> but not "git", which you need to download sources.
>

> Protip: you can catch issues like this (where your
> coincidentally-installed programs are actually needed to build) by
> checking that your package still builds in a clean chroot using the
> mkarchroot and makechrootpkg commands from [extra]/devtools. Or use the
> extra-x86_64-build convenience wrapper.
>

> > source=("git+${url}")
>

> I like it when people use variables just because they exist, so keep up
> the good work!
>

> Also this underlines the association between the url and the source
> download.
>

> > md5sums=('SKIP')
> > build() {
> >   cd "${pkgname}/src"
> > qmake
> >   make
> > }
> > package() {
> >   cd "${srcdir}/${pkgname}/src"
> >   make INSTALL_ROOT="${pkgdir}" install
> > install -Dm644 icon.png "${pkgdir}/usr/share/icons/${pkgname}/icon.png"
> >   install -Dm644 ../threemawebqt.desktop
> > "${pkgdir}/usr/share/applications/threemawebqt.desktop"
> >   install -Dm644 ../LICENSE
> > "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
> > }
>

> Since you're the upstream developer, you have the opportunity to fix
> your qmake configuration.
>

> It doesn't respect any sort of --prefix option, but hardcodes
> /usr/local/bin/ as the install location. You can have this determined at
> qmake time using e.g.
> https://stackoverflow.com/questions/7106442/qt-project-files-and-prefix-variable/7106823#7106823
>

> It should not require one to install the desktop file and icon by hand,
> but add these as additional "thing.files".
>

> The license file is not required to run, and distributions often have
> different ideas about where to install it for distribution packages...
> so you don't need to install it too.
>

> ...
>

> Correspondingly, the desktop file should not hardcode the Exec path, but
> rely on it being in the system $PATH. Likewise, the Icon should not be
> /usr/share/icons/threemawebqt/icon.png but instead "threemawebqt", and
> you should install icon.png as "threemawebqt.png" to a directory that
> obeys the XDG icon theme specification:
> https://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#directory_layout
>

> e.g. /usr/share/pixmaps/ or /usr/share/icons/hicolor/128x128/apps/
>

> ...
>

> Also your *_DIR setup uses the parent directory but assumes that it is
> run from the src/ folder, look what happens if you try to run
>

> cd /tmp
> git clone https://github.com/ethanrakoff/threemawebqt /tmp/threemawebqt
> cd /tmp/threemawebqt
> qmake src/threemawebqt.pro
> make
> ls /tmp/build
>

> I'd suggest having your .pro file in the repository root, and adding
> SOURCES as src/main.cpp then building in build rather than ../build
>

> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>

> Eli Schwartz
> Bug Wrangler and Trusted User


signature.asc (523 bytes) Download Attachment