Re: [aur-general] package review request: gog-pyre

[JereBear via aur-general]

> BTW I did submit
> https://lists.archlinux.org/pipermail/aur-dev/2019-January/004619.html
> which I think should fix this.

Thanks!

Re: [aur-general] package review request: gog-pyre

[Eli Schwartz via aur-general]

On 1/7/19 1:20 PM, JereBear wrote:
>> Also I think all previous cases of people using local:// in a PKGBUILD
>> did not use "/filename", so we were happily parsing it as an authority.
>> You've actually broken the aurweb in a practical way by actually caring
>> about the RFC. :D
> 
> That's gratifying!
> 
> This bug appears to affect only the aurweb's UI, and not makepkg, so I
> think it's safe to leave the package as-is.
> 
> I'm going to re-read the relevant RFCs over the next few days to make
> sure I'm not barking up the wrong tree.

BTW I did submit
https://lists.archlinux.org/pipermail/aur-dev/2019-January/004619.html
which I think should fix this.

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] package review request: gog-pyre

[JereBear via aur-general]

> Also I think all previous cases of people using local:// in a PKGBUILD
> did not use "/filename", so we were happily parsing it as an authority.
> You've actually broken the aurweb in a practical way by actually caring
> about the RFC. :D

That's gratifying!

This bug appears to affect only the aurweb's UI, and not makepkg, so I
think it's safe to leave the package as-is.

I'm going to re-read the relevant RFCs over the next few days to make
sure I'm not barking up the wrong tree.

Re: [aur-general] package review request: gog-pyre

[Eli Schwartz via aur-general]

On 1/6/19 2:00 PM, JereBear wrote:
>>> Thanks for the info about the local pseudo-scheme. I've made a note to
>>> file a feature request on bugs.archlinux.org, asking for documentation.
>>> (Obligatory "and maybe a patch, but don't count on it.") And I'll switch to
>>> using it.
>>
>> Documentation is always good! You're not the first one to ask about
>> where local:// is documented, I think, but documentation issues are
>> difficult to remember without a bug report when the people most likely
>> to be writing documentation think it is obvious because they've read the
>> implementation. :p
> 
> https://bugs.archlinux.org/task/61294

I noticed another bug when looking at your referenced package.

You used local:/// rather than local://, but aurweb displays this as
relative to the cgit repository. Since we use php's parse_url() to check
if it validates as having an

Array
(
[scheme] => something
)

- but, parse_url() will special-case file:// in order to treat it
  correctly, and otherwise
- parse local://filename as a schema+host, and
- parse local:///filename as malformed ("On seriously malformed URLs,
  parse_url() may return FALSE.")

So this completely borks if scheme:/// is used and treats it as local to
the cgit repository as the fallback for plain files, since it cannot get
an $array['scheme'] from a boolean FALSE. It doesn't look like
parse_url() is very suitable here... makepkg does not care either way,
since it looks for the final pathname component by discarding everything
before the last "/".

This is an aurweb bug, as I think we should abort on malformed urls and
display them as-is, like we already do for urls that are on the RHS of
makepkg's "::" syntax. Or possibly use the same dumb parser we use in
the db backend for consistency's sake...

...

Also I think all previous cases of people using local:// in a PKGBUILD
did not use "/filename", so we were happily parsing it as an authority.
You've actually broken the aurweb in a practical way by actually caring
about the RFC. :D

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] package review request: gog-pyre

[JereBear via aur-general]

> > Thanks for the info about the local pseudo-scheme. I've made a note to
> > file a feature request on bugs.archlinux.org, asking for documentation.
> > (Obligatory "and maybe a patch, but don't count on it.") And I'll switch to
> > using it.
>
> Documentation is always good! You're not the first one to ask about
> where local:// is documented, I think, but documentation issues are
> difficult to remember without a bug report when the people most likely
> to be writing documentation think it is obvious because they've read the
> implementation. :p

https://bugs.archlinux.org/task/61294

Re: [aur-general] package review request: gog-pyre

[Eli Schwartz via aur-general]

On 1/4/19 10:45 PM, JereBear wrote:
> Interesting. In the strictest case, the first phase of parsing the file scheme
> could consist of matching ^file:(\/\/([a-z0-9-]+)?)? and providing default
> values for missing capture groups. But this only makes sense if the file
> scheme is, well, to be supported. I'll think on this.

Also this would need to be supported in makepkg first, before adding it
to aurweb. aurweb is written in python, but makepkg is written in bash.

> Thanks for the info about the local pseudo-scheme. I've made a note to
> file a feature request on bugs.archlinux.org, asking for documentation.
> (Obligatory "and maybe a patch, but don't count on it.") And I'll switch to
> using it.

Documentation is always good! You're not the first one to ask about
where local:// is documented, I think, but documentation issues are
difficult to remember without a bug report when the people most likely
to be writing documentation think it is obvious because they've read the
implementation. :p

>> makechrootpkg is designed to run makepkg in a highly controlled
>> environment suitable for official packaging. It's undesirable for
>> makepkg settings like PKGEXT to leak from a developer's machine into the
>> environment used for uploading packages to the official repos.
>>
>> If you need something else, then don't use extra-x86_64-build, and
>> instead mkarchroot with the makepkg.conf you want to initialize into the
>> chroot. Or use arch-nspawn -M  to copy your makepkg.conf over the
>> next time you do chroot administration.
> 
> Ah-ha! It's good to know the target use case for extra-x86_64-build and
> makechrootpkg. And I hadn't thought about using separate chroots for public vs
> private builds, or inserting files with arch-nspawn -M $file. Good stuff.

the different tools have lots of cool options. ;)

makechrootpkg lets you use existing chroots configurably created with
mkarchroot, extra-x86_64-build is just a convenience script designed for
the official repos which:
- handles mkarchroot automatically
- updates the root/ chroot
- hardcodes official defaults that are usually suitable for users too

The aurutils AUR helper does contain its own `aur build` wrapper which
uses mkarchroot and makechrootpkg directly, while also exposing
makepkg.conf as an option.

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature

Re: [aur-general] package review request: gog-pyre

[JereBear via aur-general]

> If you like, you can report a bug to the aurweb parser arguing that
> you'd like to use your RFC 3986 right to an empty URI authority in
> combination with a permissive file: scheme. But currently, attempting to
> push that to the AUR will result in failure due to rejecting any source
> that does not contain the string literal :// and is also not committed
> to the git repository.
>
> Furthermore, makepkg itself does the same thing, and considers
> file:/path to be a relative filesystem path for "./file:/path", which,
> since makepg does not support subdirectories (whether that subdirectory
> name has a ":" in it in a manner reminiscent of RFC 3986 is not
> relevant) the leading directory will simply be dropped at every point
> where makepkg attempts to parse it.
>
> ...
>
> personally I don't see much use for skipping the ://, file:/// works
> everywhere and is more easily parsed.
>
> ...
>
> Since the only reason to use any scheme at all is to pass validation for
> aurweb, you won't notice anything wrong with makepkg.
>
> As for local://, it is special-cased in makepkg -- if makepkg sees it,
> it will not try to handle it as a download url no matter what. You can
> get a similar result by using sdfsgsgfdiuyicl:// since makepkg does most
> likely not have a DLAGENTS=() handler for this and will therefore tell
> you it cannot figure out how to download it... but it's the difference
> between "unknown download client" and "was not found in the build
> directory and is not a URL."

Interesting. In the strictest case, the first phase of parsing the file scheme
could consist of matching ^file:(\/\/([a-z0-9-]+)?)? and providing default
values for missing capture groups. But this only makes sense if the file
scheme is, well, to be supported. I'll think on this.

Thanks for the info about the local pseudo-scheme. I've made a note to
file a feature request on bugs.archlinux.org, asking for documentation.
(Obligatory "and maybe a patch, but don't count on it.") And I'll switch to
using it.

> > Shall I leave both of the variable assignments commented out, so that
> > makepkg.conf is respected, and so that the user is aware of possible
> > solutions? Or do you think it's better practice to omit such comments
> > altogether?
>
> I'm of the opinion people should read the documentation and set
> PKGEXT=.pkg.tar as described here:
> https://wiki.archlinux.org/index.php/Makepkg#Use_other_compression_algorithms
>
> Or the next tip which shows how to modify COMPRESSXZ

I'll insert a comment linking to the wiki. Anticipates user needs while
remaining DRY.

> makechrootpkg is designed to run makepkg in a highly controlled
> environment suitable for official packaging. It's undesirable for
> makepkg settings like PKGEXT to leak from a developer's machine into the
> environment used for uploading packages to the official repos.
>
> If you need something else, then don't use extra-x86_64-build, and
> instead mkarchroot with the makepkg.conf you want to initialize into the
> chroot. Or use arch-nspawn -M  to copy your makepkg.conf over the
> next time you do chroot administration.

Ah-ha! It's good to know the target use case for extra-x86_64-build and
makechrootpkg. And I hadn't thought about using separate chroots for public vs
private builds, or inserting files with arch-nspawn -M $file. Good stuff.

> That's quite sad, but if there's no way in makeself to extract the
> assets zip without using mojosetup, then I guess you're doing the best
> you can.

I legitimately think stripping bytes is the best option.

> > symlinking to start.sh is not an option here. :-( Here's the contents of 
> > gog-pyre:
> >
> > #!/usr/bin/env bash
> >
> > # Setting TERM prevents the application from crashing with 
> > "System.Exception:
> > # Magic number is wrong: 542." See: 
> > https://github.com/mono/mono/issues/11557
> > TERM=xterm /opt/gog-pyre/start.sh
> >
> > I suspect that this workaround is only necessary because Pyre bundles 
> > dependencies.
>
> Oh well, it was worth a thought -- but next time you can post the script
> as well as the PKGBUILD and we'll know what it's doing right off the bat. ;)

Yeah, I didn't think this through.

Re: [aur-general] package review request: gog-pyre

[Eli Schwartz via aur-general]

On 1/4/19 1:32 PM, JereBear wrote:
>>>
>>> source=("file:/pyre_${pkgver//./_}.sh"
>>
>> This is not file:// because it is typoed.
>>
>> But you should use local:// since file:// is an actual makepkg.conf
>> protocol and will be downloaded using curl from an on-disk location.
> 
> I'll switch to using the local schema. Is there some resource, such as a man 
> page, which explains it?
> 
> "file:/" is intentional, and not a typo. From Wikipedia:
> 
> file://path (i.e. two slashes, without a hostname) is never correct, but 
> is often used.
> 
> See: https://en.wikipedia.org/wiki/File_URI_scheme#How_many_slashes?
If you like, you can report a bug to the aurweb parser arguing that
you'd like to use your RFC 3986 right to an empty URI authority in
combination with a permissive file: scheme. But currently, attempting to
push that to the AUR will result in failure due to rejecting any source
that does not contain the string literal :// and is also not committed
to the git repository.

Furthermore, makepkg itself does the same thing, and considers
file:/path to be a relative filesystem path for "./file:/path", which,
since makepg does not support subdirectories (whether that subdirectory
name has a ":" in it in a manner reminiscent of RFC 3986 is not
relevant) the leading directory will simply be dropped at every point
where makepkg attempts to parse it.

...

personally I don't see much use for skipping the ://, file:/// works
everywhere and is more easily parsed.

...

Since the only reason to use any scheme at all is to pass validation for
aurweb, you won't notice anything wrong with makepkg.

As for local://, it is special-cased in makepkg -- if makepkg sees it,
it will not try to handle it as a download url no matter what. You can
get a similar result by using sdfsgsgfdiuyicl:// since makepkg does most
likely not have a DLAGENTS=() handler for this and will therefore tell
you it cannot figure out how to download it... but it's the difference
between "unknown download client" and "was not found in the build
directory and is not a URL."

> Shall I leave both of the variable assignments commented out, so that
> makepkg.conf is respected, and so that the user is aware of possible
> solutions? Or do you think it's better practice to omit such comments
> altogether?

I'm of the opinion people should read the documentation and set
PKGEXT=.pkg.tar as described here:
https://wiki.archlinux.org/index.php/Makepkg#Use_other_compression_algorithms

Or the next tip which shows how to modify COMPRESSXZ

> 
> I've not yet found a technique for configuring makepkg when building in a 
> chroot with makechrootpkg. Here's the command I'm currently using:
> 
> makechrootpkg \
> -c \
> -I /var/cache/pacman/pkg/binkplayer-bin-2.7J-1-x86_64.pkg.tar.xz \
> -r /var/lib/archbuild/extra-x86_64/
> 
> That's what prompted me to insert these variable assignments into PKGBUILD in 
> the first place, rather than just advising the user to configure 
> makepkg.conf. I'm happy to read a man page or wiki page if there's advice on 
> this topic.
> 
> It's too bad that multi-threaded compression is incompatible with 
> reproducible builds. :-(

makechrootpkg is designed to run makepkg in a highly controlled
environment suitable for official packaging. It's undesirable for
makepkg settings like PKGEXT to leak from a developer's machine into the
environment used for uploading packages to the official repos.

If you need something else, then don't use extra-x86_64-build, and
instead mkarchroot with the makepkg.conf you want to initialize into the
chroot. Or use arch-nspawn -M  to copy your makepkg.conf over the
next time you do chroot administration.

>>> prepare() {
>>>   snip!
>>> }
>>
>> This looks very complicated, shouldn't there be a way to tell it to
>> extract to a temporary directory? I'm not overly familiar with makeself
>> installers though.
> 
> Your intuition is correct: makeself can extract the blob that it prepends. 
> This command will do so:
> 
> pyre-${pkgver//./_}.sh --keep --target data
> 
> But GOG games consist of a makeself script, mojosetup installer, and a zip 
> archive containing the game assets, concatenated in that order. As a result, 
> the command above extracts the *mojosetup* installer, not the game itself. 
> Meanwhile, the mojosetup installer is an interactive GUI application that 
> installs the game into the calling user's home directory. Not what I want. As 
> a fall-back solution, I'm stripping the makeself script and mojosetup 
> installer with tail. In the simplest case, it looks like the following:
> 
> tail --bytes=+757175 pyre-${pkver//./_}.sh > pyre.zip
> unzip -d pyre pyre.zip
> 
> However, I dislike hard-coding the number of bytes to strip, and I dislike 
> calling unzip and hoping for the best, so most of prepare() is concerned with 
> calculating the number of bytes to strip. I'm happy to explain the byte 
> calculation logic in detail if you like.
> 
> I'd 

Re: [aur-general] package review request: gog-pyre

[JereBear via aur-general]

> >
> > source=("file:/pyre_${pkgver//./_}.sh"
>
> This is not file:// because it is typoed.
>
> But you should use local:// since file:// is an actual makepkg.conf
> protocol and will be downloaded using curl from an on-disk location.

I'll switch to using the local schema. Is there some resource, such as a man 
page, which explains it?

"file:/" is intentional, and not a typo. From Wikipedia:

file://path (i.e. two slashes, without a hostname) is never correct, but is 
often used.

See: https://en.wikipedia.org/wiki/File_URI_scheme#How_many_slashes?

> > # Single-threaded compression of a multi-gigabyte package is time-consuming.
> > # Possible solutions are to skip compression or throw more threads at it.
> > # PKGEXT='.pkg.tar'
> > COMPRESSXZ=(xz --to-stdout --compress --threads 0 -)
>
> Users who want to avoid compressing packages because they either don't
> need the storage savings or aren't hosting them over a network, should
> set PKGEXT in their makepkg.conf -- it's not the job of the package to
> enforce this.
>
> Meanwhile, single-threaded compression is the default in makepkg because
> multithreaded compression results in packages that cannot be
> reproducibly built. Even decompressing and recompressing the tarball
> will result in changes, so it would also break delta packages.

Shall I leave both of the variable assignments commented out, so that 
makepkg.conf is respected, and so that the user is aware of possible solutions? 
Or do you think it's better practice to omit such comments altogether?

I've not yet found a technique for configuring makepkg when building in a 
chroot with makechrootpkg. Here's the command I'm currently using:

makechrootpkg \
-c \
-I /var/cache/pacman/pkg/binkplayer-bin-2.7J-1-x86_64.pkg.tar.xz \
-r /var/lib/archbuild/extra-x86_64/

That's what prompted me to insert these variable assignments into PKGBUILD in 
the first place, rather than just advising the user to configure makepkg.conf. 
I'm happy to read a man page or wiki page if there's advice on this topic.

It's too bad that multi-threaded compression is incompatible with reproducible 
builds. :-(

> > prepare() {
> >   snip!
> > }
>
> This looks very complicated, shouldn't there be a way to tell it to
> extract to a temporary directory? I'm not overly familiar with makeself
> installers though.

Your intuition is correct: makeself can extract the blob that it prepends. This 
command will do so:

pyre-${pkgver//./_}.sh --keep --target data

But GOG games consist of a makeself script, mojosetup installer, and a zip 
archive containing the game assets, concatenated in that order. As a result, 
the command above extracts the *mojosetup* installer, not the game itself. 
Meanwhile, the mojosetup installer is an interactive GUI application that 
installs the game into the calling user's home directory. Not what I want. As a 
fall-back solution, I'm stripping the makeself script and mojosetup installer 
with tail. In the simplest case, it looks like the following:

tail --bytes=+757175 pyre-${pkver//./_}.sh > pyre.zip
unzip -d pyre pyre.zip

However, I dislike hard-coding the number of bytes to strip, and I dislike 
calling unzip and hoping for the best, so most of prepare() is concerned with 
calculating the number of bytes to strip. I'm happy to explain the byte 
calculation logic in detail if you like.

I'd happily use pipes so as to avoid a large temporarily zip file:

tail --bytes=+757175 pyre-${pkver//./_}.sh | unzip -d pyre

...but unzip can't read from stdin.

> > package() {
> > # game files and launcher
> > install -d "${pkgdir}/opt/${pkgname}"
> > cp -rt "${pkgdir}/opt/${pkgname}" "${srcdir}/pyre/data/noarch/"*
> > chmod 755 "${pkgdir}/opt/${pkgname}/start.sh"
> > install -Dm755 "${srcdir}/${pkgname}" "${pkgdir}/usr/bin/${pkgname}"
>
> What does this script do? Most such script launchers (start.sh) should
> in theory work okay if you symlink them to /usr/bin, is this not an
> option here?

symlinking to start.sh is not an option here. :-( Here's the contents of 
gog-pyre:

#!/usr/bin/env bash

# Setting TERM prevents the application from crashing with 
"System.Exception:
# Magic number is wrong: 542." See: 
https://github.com/mono/mono/issues/11557
TERM=xterm /opt/gog-pyre/start.sh

I suspect that this workaround is only necessary because Pyre bundles 
dependencies.

> > chmod ugo+w "${pkgdir}/opt/${pkgname}/game/DebugLog.txt"
>
> What is this debuglog needed for? I wonder if it would be better to
> symlink it to /dev/stdout

I only glanced at the log. IIRC, it contains information like the current 
graphics card. I'll try symlinking to /dev/stdout. That seems like a much 
better idea than creating a world-writable file, and it's more useful than 
symlinking to /dev/null.

> Eli Schwartz
> Bug Wrangler and Trusted User

Thank you for your time!

Re: [aur-general] package review request: gog-pyre

[Eli Schwartz via aur-general]

On 1/4/19 11:21 AM, JereBear via aur-general wrote:
> Would someone be willing to review the gog-pyre package I've put together? 
> I've included just the PKGBUILD below, but I'm happy to include the entire 
> enchilada with git-format-patch. IIRC, this mailing list strips attachments, 
> so I've included the PKGBUILD inline. I've successfully compiled and 
> installed the package, and can verify that the application works. (At least, 
> the first five minutes of gameplay work.)
> 
> 
> 
> 
> 
> # Maintainer: Jeremy Audet 
> # shellcheck shell=bash
> # shellcheck disable=SC2034,SC2154
> 
> pkgname='gog-pyre'
> pkgver='1.50427.11957.23366'
> pkgrel=1
> pkgdesc='A party-based RPG in which you lead a band of exiles to freedom. 
> (GOG version)'
> url='https://www.gog.com/game/pyre'
> license=('custom')
> groups=('games' 'gog')
> # May also work on i686, but untested. Most 64-bit dependencies (e.g. sdl2) 
> are
> # bundled with the game.
> arch=('x86_64')
> depends=('binkplayer-bin')
> makedepends=('unzip')
> # Copy the game file into the current directory before building.
> source=("file:/pyre_${pkgver//./_}.sh"

This is not file:// because it is typoed.

But you should use local:// since file:// is an actual makepkg.conf
protocol and will be downloaded using curl from an on-disk location.

> "${pkgname}.desktop"
> "${pkgname}")
> sha256sums=('f42b4c55975df69e8d98069dea72178320485f6bb8b8a1573490e10331fa17d2'
> 'f66a8ad19f05d826afbe2a9375d1b6317f9166dc304ba1be2b92913064bf6971'
> 
> '5b91e71101efe303986851df828d2e3934715232bfc12bb3634d1ec49cf70e42')
> 
> # Single-threaded compression of a multi-gigabyte package is time-consuming.
> # Possible solutions are to skip compression or throw more threads at it.
> # PKGEXT='.pkg.tar'
> COMPRESSXZ=(xz --to-stdout --compress --threads 0 -)

Users who want to avoid compressing packages because they either don't
need the storage savings or aren't hosting them over a network, should
set PKGEXT in their makepkg.conf -- it's not the job of the package to
enforce this.

Meanwhile, single-threaded compression is the default in makepkg because
multithreaded compression results in packages that cannot be
reproducibly built. Even decompressing and recompressing the tarball
will result in changes, so it would also break delta packages.

> prepare() {
>   # GOG games for linux appear to consist of a concatenated makeself shell
>   # script, mojosetup gzip archive, and game zip archive. I've not found a way
>   # to make the scripts strip themselves, and use tail as a fall-back. A hacky
>   # way of verifying $header_bytes is to call unzip on the game file, and to 
> see
>   # how many bytes it skips while searching for the start of the zip archive.
>   bash "pyre_${pkgver//./_}.sh" --dumpconf > conf
> 
>   local makeself_lines
>   # shellcheck disable=SC1091
>   makeself_lines="$(( "$(source conf && echo "${OLDSKIP}")" - 1 ))"
> 
>   local makeself_bytes
>   makeself_bytes="$(
> head "pyre_${pkgver//./_}.sh" --lines "${makeself_lines}" \
> | wc --bytes
>   )"
> 
>   local mojosetup_bytes
>   # shellcheck disable=SC1091
>   mojosetup_bytes="$(source conf && echo "${filesizes}")"
> 
>   local header_bytes
>   header_bytes="$((makeself_bytes + mojosetup_bytes))"
> 
>   tail --bytes=+"$((header_bytes + 1))" "pyre_${pkgver//./_}.sh" > pyre.zip
>   unzip -qd pyre pyre.zip
> }

This looks very complicated, shouldn't there be a way to tell it to
extract to a temporary directory? I'm not overly familiar with makeself
installers though.

> package() {
>   # game files and launcher
>   install -d "${pkgdir}/opt/${pkgname}"
>   cp -rt "${pkgdir}/opt/${pkgname}" "${srcdir}/pyre/data/noarch/"*
>   chmod 755 "${pkgdir}/opt/${pkgname}/start.sh"
>   install -Dm755 "${srcdir}/${pkgname}" "${pkgdir}/usr/bin/${pkgname}"

What does this script do? Most such script launchers (start.sh) should
in theory work okay if you symlink them to /usr/bin, is this not an
option here?

>   # The game writes to DebugLog.txt, which results in a permission denied 
> error.
>   # AFAICT, this is a bug, and it should write to a location suited for the
>   # purpose, such as a subdirectory of the calling user's $XDG_CACHE_HOME. A
>   # simple solution is to symlink DebugLog.txt to /dev/null. Making 
> DebugLog.txt
>   # world-writable is less secure but more useful.
>   chmod ugo+w "${pkgdir}/opt/${pkgname}/game/DebugLog.txt"

What is this debuglog needed for? I wonder if it would be better to
symlink it to /dev/stdout

>   # desktop environment integration
>   install -Dm644 \
> "${srcdir}/pyre/data/noarch/support/icon.png" \
> "${pkgdir}/usr/share/pixmaps/${pkgname}.png"
>   install -Dm644 \
> "${srcdir}/${pkgname}.desktop" \
> "${pkgdir}/usr/share/applications/${pkgname}.desktop"
> 
>   # license
>   install -Dm644 \
> "${srcdir}/pyre/data/noarch/docs/End User License Agreement.txt" \
> "${pkgdir}/usr/share/licenses/${pkgname}/LICENSE"
> }
> 
> # 

Re: [aur-general] Package review request

[Stefan Auditor via aur-general]

On Wed, 2017-03-15 at 10:10 -0500, Doug Newgard wrote:
> On Wed, 15 Mar 2017 15:41:27 +0100
> Stefan Auditor via aur-general  wrote:
> 
> > Hi list,
> > 
> > creating a PKGBUILD[1] for Traefik[2], a reverse proxy, I would
> > like to
> > have it reviewed, as this is the first package I do that is
> > containing
> > a service.
> > 
> > I tried following the Arch packaging standards[3] but am fairly
> > sure I
> > missed things.
> > 
> > Other than that it's a Golang project, so I just have a binary and
> > some
> > configuration files to package.
> > 
> > Thank you for your time.
> > Stefan
> > 
> > [1] https://aur.archlinux.org/packages/traefik-bin/
> > [2] https://github.com/containous/traefik
> > [3] https://wiki.archlinux.org/index.php/Arch_packaging_standards
> 
> The biggest problem is that is will fail on a lot of setups. You
> don't have the
> service, logrotate file, etc in the source array and assume that
> they're one
> dir up from $srcdir. This is wrong.
Changed that to include all files that are to be installed.

> Why do you have the LICENSE.md in both source_* arrays instead of the
> normal
> source array? You should also not be pulling that from master, as you
> don't
> know when that will change. It should be put in a subdir of $pkgname,
> not
> $_pkgname.
Changed that, too.

> chowning a managed dir in post_install is less than idea. You should
> be using a
> set UID/GID and chowing it in the package function.
Tried chowning in the package function, but as the user/group doesn't
exist when packaging, it fails. Also found some packages[1][2] that are
having the same problem and do it in the post_install hook.
So, sticking to that by now

> You should also not be
> deleting the user in post_remove.
Removed that.

> Hope this helps!
Yes, thanks alot!

[1] 
https://git.archlinux.org/svntogit/community.git/tree/trunk/netdata.install?h=packages/netdata
[2] https://aur.archlinux.org/cgit/aur.git/tree/prometheus.install?h=prometheus

signature.asc
Description: This is a digitally signed message part

Re: [aur-general] Package review request

[Doug Newgard]

On Wed, 15 Mar 2017 15:41:27 +0100
Stefan Auditor via aur-general  wrote:

> Hi list,
> 
> creating a PKGBUILD[1] for Traefik[2], a reverse proxy, I would like to
> have it reviewed, as this is the first package I do that is containing
> a service.
> 
> I tried following the Arch packaging standards[3] but am fairly sure I
> missed things.
> 
> Other than that it's a Golang project, so I just have a binary and some
> configuration files to package.
> 
> Thank you for your time.
> Stefan
> 
> [1] https://aur.archlinux.org/packages/traefik-bin/
> [2] https://github.com/containous/traefik
> [3] https://wiki.archlinux.org/index.php/Arch_packaging_standards

The biggest problem is that is will fail on a lot of setups. You don't have the
service, logrotate file, etc in the source array and assume that they're one
dir up from $srcdir. This is wrong.

Why do you have the LICENSE.md in both source_* arrays instead of the normal
source array? You should also not be pulling that from master, as you don't
know when that will change. It should be put in a subdir of $pkgname, not
$_pkgname.

chowning a managed dir in post_install is less than idea. You should be using a
set UID/GID and chowing it in the package function. You should also not be
deleting the user in post_remove.

Hope this helps!


pgptPLcZZWHwe.pgp
Description: OpenPGP digital signature