Package Review

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

Package Review

Joaquin Garmendia Cabrera
Hello!,

Today, I adopted a little package and update some things that weren't
according to packaging standards. I would be very grateful if somebody
could suggest improvements.
PKGBUILD is in [1].

Best Regards,
Joaquin

[1]
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=gtk-arc-flatabulous-theme-git
Reply | Threaded
Open this post in threaded view
|

Re: Package Review

tur-users mailing list
On 01/07/2018 02:59 PM, Joaquin Garmendia Cabrera wrote:

> Hello!,
>
> Today, I adopted a little package and update some things that weren't
> according to packaging standards. I would be very grateful if somebody
> could suggest improvements.
> PKGBUILD is in [1].
>
> Best Regards,
> Joaquin
>
> [1]
> https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=gtk-arc-flatabulous-theme-git
Your first change was to create a merge commit from your own git repo,
rather than remove all files from the package repo and replace them with
your own. This created a merge commit of two unrelated git repositories,
which is kind of messy and makes it difficult to see what actually happened.

Other than that you basically just moved some dependencies to
optdepends. This seems fair as gtk3 is needed for building but other
than that they're just a bunch of data files... but surely gtk3 should
be an optdepends too, then?
You also removed the useless makedepends on packages in the base-devel
group, so good job on that!

Regarding pre-existing issues which should probably be fixed:

1) You can simplify to `source=("git+${url}")` as the default download
name is already the last component of the url, which coincidentally is
*also* the value of ${_pkgname}. ;)

2) Instead of using ./autogen.sh in build, use the standard tool
`autoreconf -fi`, which autotools build systems are supposed to migrate
to. This one actually does use autoreconf, but keeps a legacy autogen.sh
which just runs autoreconf -fi and then runs configure automatically.
Note that `autoreconf -fi` should properly be run in prepare() and then
`./configure --prefix=/usr` should be separately run in build()

3) The pkgver currently uses:
echo "$(git rev-list --count HEAD).$(git rev-parse --short HEAD)"

But ideally it would be prepended with an "r", as such:
printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse --short
HEAD)"

See https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git for
details and general guidance. The reason for this, is because if the
repository ever gains tags, then it will be easy to upgrade from
r709.1b1b7c9 to 1.0 or whatever, because the r is automatically less
than 1. However, 709 is far more than 1, so there is now no way to
upgrade to tags without either causing users to "downgrade" or adding
the PKGBUILD "epoch" keyword when updating to a newly incompatible
version schema.

There is unfortunately no nice way to fix this. So I would leave it
as-is. If upstream ever adds tagged releases, you will need to update
the pkgver() to use the tags as per the wiki instructions, and
additionally add an `epoch=1` after the pkgver and pkgrel.

--
Eli Schwartz


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

Re: Package Review

Joaquin Garmendia Cabrera
I just updated the pkgbuild following your recommendations. I just have a
question: Why not to put `autoreconf -fi` and `configure --prefix=/usr`
under prepare() function?

Thanks for your time!,
Joaquin

2018-01-07 16:12 GMT-05:00 Eli Schwartz via aur-general <
[hidden email]>:

> On 01/07/2018 02:59 PM, Joaquin Garmendia Cabrera wrote:
> > Hello!,
> >
> > Today, I adopted a little package and update some things that weren't
> > according to packaging standards. I would be very grateful if somebody
> > could suggest improvements.
> > PKGBUILD is in [1].
> >
> > Best Regards,
> > Joaquin
> >
> > [1]
> > https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=
> gtk-arc-flatabulous-theme-git
>
> Your first change was to create a merge commit from your own git repo,
> rather than remove all files from the package repo and replace them with
> your own. This created a merge commit of two unrelated git repositories,
> which is kind of messy and makes it difficult to see what actually
> happened.
>
> Other than that you basically just moved some dependencies to
> optdepends. This seems fair as gtk3 is needed for building but other
> than that they're just a bunch of data files... but surely gtk3 should
> be an optdepends too, then?
> You also removed the useless makedepends on packages in the base-devel
> group, so good job on that!
>
> Regarding pre-existing issues which should probably be fixed:
>
> 1) You can simplify to `source=("git+${url}")` as the default download
> name is already the last component of the url, which coincidentally is
> *also* the value of ${_pkgname}. ;)
>
> 2) Instead of using ./autogen.sh in build, use the standard tool
> `autoreconf -fi`, which autotools build systems are supposed to migrate
> to. This one actually does use autoreconf, but keeps a legacy autogen.sh
> which just runs autoreconf -fi and then runs configure automatically.
> Note that `autoreconf -fi` should properly be run in prepare() and then
> `./configure --prefix=/usr` should be separately run in build()
>
> 3) The pkgver currently uses:
> echo "$(git rev-list --count HEAD).$(git rev-parse --short HEAD)"
>
> But ideally it would be prepended with an "r", as such:
> printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse --short
> HEAD)"
>
> See https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git for
> details and general guidance. The reason for this, is because if the
> repository ever gains tags, then it will be easy to upgrade from
> r709.1b1b7c9 to 1.0 or whatever, because the r is automatically less
> than 1. However, 709 is far more than 1, so there is now no way to
> upgrade to tags without either causing users to "downgrade" or adding
> the PKGBUILD "epoch" keyword when updating to a newly incompatible
> version schema.
>
> There is unfortunately no nice way to fix this. So I would leave it
> as-is. If upstream ever adds tagged releases, you will need to update
> the pkgver() to use the tags as per the wiki instructions, and
> additionally add an `epoch=1` after the pkgver and pkgrel.
>
> --
> Eli Schwartz
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Package Review

tur-users mailing list
On 01/07/2018 07:11 PM, Joaquin Garmendia Cabrera wrote:
> I just updated the pkgbuild following your recommendations. I just have a
> question: Why not to put `autoreconf -fi` and `configure --prefix=/usr`
> under prepare() function?
>
> Thanks for your time!,
> Joaquin

Strictly speaking, the prepare() function is meant to perform final
changes to the source code in order to get it ready for building. This
includes things like applying patches or sed commands, as well as
autoreconf as autoreconf is used to create "dist" tarballs. It copies
various m4 macros into the source tree, and generates the ./configure
script as well as various other bits and pieces of an autotools system,
and the final result is a source tree that is ready for distribution as
"ready to build", since it has no need of external tools like aclocal,
autoconf, autoheader, automake etc.

In comparison, ./configure is where the packaging starts. You use
./configure to specify things for *your* specific build, e.g. the
--prefix, enabling and disabling optional features, toggling out-of-tree
builds.

Semantically, autoreconf/autogen.sh are very much preparatory tools,
whereas /.configure is very much a build tool. I like to be exact about
things like this.

--
Eli Schwartz


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

Re: Package Review

Joaquin Garmendia Cabrera
I understand your point and think it is correct.
I was learning about autotools today and I have a better view of the
situation.
I was confused as well because in some places I readed the analogy:
           `./configure` -> prepare(), `make` -> build() and `make install`
- > package().

Thanks for the help!.
Joaquin