You are not logged in.

#1 2019-01-21 09:03:17

nevdelap
Member
Registered: 2019-01-21
Posts: 6

PKGBUILD review request.

This is my first one. Thanking you in advance for your advice. Nev

https://github.com/nevdelap/ned-aur

This is where the ned project is, and the issue where I'm tracking adding this package to the AUR.

https://github.com/nevdelap/ned/issues/60

Last edited by nevdelap (2019-01-21 09:12:11)

Offline

#2 2019-01-21 11:15:29

schard
Member
From: Hannover
Registered: 2016-05-06
Posts: 1,932
Website

Re: PKGBUILD review request.

Some points:
1) Obfuscate your email address when hosting code, if you don't want to get spammed.
2) Exclude empty variable assignments such as "foo=" or "bar=()" from the PKGBUILD, as they are defaults.
3) Don't use "mkdir" and "cp" to install directories and files into the package tree, but prefer "install". "man install" will give you instructions on its proper usage.

Update:
4) You provide the package through your PKGBUILD licensed under "GPLv3". But the actual code on github does not mention such a license. Since you seem to be the author of the source code as well, consider adding a proper license text to the source code repository.
5) Arch Linux does not support the "i686" architecture any longer, so you might as well remove that from the "arch" array.

Last edited by schard (2019-01-21 11:20:25)

Online

#3 2019-01-21 12:00:15

Lone_Wolf
Member
From: Netherlands, Europe
Registered: 2005-10-04
Posts: 11,866

Re: PKGBUILD review request.

source=("https://github.com/nevdelap/ned/archive/release.$pkgver.tar.gz")

Many people use $SRCDEST to avoid duplicate downloads. To make this work downloaded files need to have  a unique name.

see https://wiki.archlinux.org/index.php/PKGBUILD#source


Disliking systemd intensely, but not satisfied with alternatives so focusing on taming systemd.


(A works at time B)  && (time C > time B ) ≠  (A works at time C)

Online

#4 2019-01-21 12:04:49

ugjka
Member
From: Latvia
Registered: 2014-04-01
Posts: 1,794
Website

Re: PKGBUILD review request.

IIRC strip is not necessary as makepkg will do it for you and it is a configurable action in /etc/makepkg.conf so leave it up to the user.

Last edited by ugjka (2019-01-21 12:05:40)


https://ugjka.net
paru > yay | webcord > discord
pacman -S spotify-launcher
mount /dev/disk/by-...

Offline

#5 2019-01-21 14:52:50

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 29,441
Website

Re: PKGBUILD review request.

Is this a statically linked binary after the build?  I was skeptical that there would be no dependencies, so I tried to check it out myself, but the build failed:

...
   Compiling ned v1.2.5 (/tmp/ned/src/ned-release.1.2.5)
error: Could not compile `ned`.

Caused by:
  process didn't exit successfully: `rustc --crate-name ned src/main.rs --color always --crate-type bin --emit=dep-info,link -C opt-level=3 -C lto -C metadata=c3275616fc5fc9d9 -C extra-filename=-c3275616fc5fc9d9 --out-dir /tmp/ned/src/ned-release.1.2.5/target/release/deps -L dependency=/tmp/ned/src/ned-release.1.2.5/target/release/deps --extern ansi_term=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libansi_term-1f5ed48093500bbf.rlib --extern getopts=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libgetopts-00fc3bd400d2cbec.rlib --extern glob=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libglob-e086e0d80c192087.rlib --extern libc=/tmp/ned/src/ned-release.1.2.5/target/release/deps/liblibc-7758dec3c48cc996.rlib --extern regex=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libregex-bd4c351cabf0749e.rlib --extern time=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libtime-ea24b810ada4b5f5.rlib --extern walkdir=/tmp/ned/src/ned-release.1.2.5/target/release/deps/libwalkdir-17330c1e965dfd9c.rlib` (signal: 11, SIGSEGV: invalid memory reference)
==> ERROR: A failure occurred in build().
    Aborting...

I know nothing about rust, and can't interpret that error message except that the build process itself seems to have seg faulted.

Last edited by Trilby (2019-01-21 14:53:50)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

#6 2019-01-21 15:07:42

Piri
Member
Registered: 2018-06-02
Posts: 70

Re: PKGBUILD review request.

You might want to add the --locked  flag to cargo build if you care about reproducible builds
https://wiki.archlinux.org/index.php/Ru … guidelines

Trilby wrote:

I know nothing about rust, and can't interpret that error message except that the build process itself seems to have seg faulted.

Seems like a bug that applies to general rust compiling.

https://bugs.archlinux.org/task/61484
https://github.com/rust-lang/rust/issues/57762

Offline

#7 2019-01-21 18:53:48

nevdelap
Member
Registered: 2019-01-21
Posts: 6

Re: PKGBUILD review request.

Wow. Thank you very much for all the feedback everyone.

Offline

#8 2019-01-21 19:45:14

Slithery
Administrator
From: Norfolk, UK
Registered: 2013-12-01
Posts: 5,776

Re: PKGBUILD review request.

schard wrote:

5) Arch Linux does not support the "i686" architecture any longer, so you might as well remove that from the "arch" array.

Unless of course you've also tested the PKGBUILD on an Arch32 installation smile


No, it didn't "fix" anything. It just shifted the brokeness one space to the right. - jasonwryan
Closing -- for deletion; Banning -- for muppetry. - jasonwryan

aur - dotfiles

Offline

#9 2019-01-21 19:53:14

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 29,441
Website

Re: PKGBUILD review request.

Indeed the 'arch' array can include a wide range of architectures that are not officially supported.  PKGBUILDs can, and not-infrequently do, list a range of arm architectures.  If x86_64 isn't in the list, then the PKGBUILD has no place in the AUR, but additional tested architectures are certainly welcome.

Last edited by Trilby (2019-01-21 19:53:32)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

#10 2019-01-21 20:26:30

nevdelap
Member
Registered: 2019-01-21
Posts: 6

Re: PKGBUILD review request.

From schard - thanks.
- obfuscate email. done
- remove empty variables. done
- use install instead of mkdir, cp. done
- add licence to ned repo - it is mentioned in usage and I've added the license to the source.
- remove i686. done

From the bottom of the ned --help usage:

License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

From Lone_Wolf - thanks.
- TODO: use $SRCDEST - figure out what it is for and how to use it.

From ugjka - thanks.
- strip not necessary - but I've added it to the install command.

Q. What would be a situation where you wouldn't want to strip?

From Trilby & Piri - thanks.
- Fixed depends - added gcc-libs.

$ namcap ned-1.2.5-1-x86_64.pkg.tar.xz
ned E: Dependency gcc-libs detected and not included (libraries ['usr/lib/libgcc_s.so.1'] needed in files ['usr/bin/ned'])

- TODO: Read those links about the segfault and figure out what to do about it.

From Piri - thanks.
- add --locked to build and test.

Last edited by nevdelap (2019-01-21 20:51:52)

Offline

#11 2019-01-21 21:02:12

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 29,441
Website

Re: PKGBUILD review request.

For GPL usage, please see here:
https://www.gnu.org/licenses/gpl-howto.en.html

Note that you need not install the 'COPYING' file in the arch package, as it is provided by the 'licenses' package already and every GPLv3 COPYING / license file is identical (this is not the case with MIT for example, for which a license file must be packaged by each package using it).

nevdelap wrote:

Q. What would be a situation where you wouldn't want to strip?

One example is that stripping hinders debugging.  But it doesn't really matter if there is a reason to not strip - or if you'd agree with an end-user's preference to not strip binaries, just leave it up to them.  This is not something you should try to force on others.  It is a configurable makepkg behavior, allow them to configure it rather than hard-coding in your preference for no reason.  (Note the default configuration is to strip binaries, so doing it yourself manually does nothing for most users who keep the default, but could aggravate users who for whatever reason specifically decided to change the default in order to not strip binaries).

nevdelap wrote:

- TODO: Read those links about the segfault and figure out what to do about it.

As your package requires the use of the rust compiler, it'd be good for you to be aware of the issue, but it is definitely not your job to deal with it.  In my case, the problem is at my end: my rust compiler isn't working properly (it is the default arch repo rust compiler, but just the same, not directly your problem).

Last edited by Trilby (2019-01-21 21:03:57)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

#12 2019-01-21 22:01:21

eschwartz
Fellow
Registered: 2014-08-08
Posts: 4,097

Re: PKGBUILD review request.

The rust compiler issue is because there was an upstream bug in llvm, and Arch Linux provides a rustc which makes use of the system installation for libLLVM.so

The bug is now fixed -- we have backported an llvm patch and rustc should now work after you successfully:

pacman -Syu "llvm-libs>=7.0.1-2"
nevdelap wrote:

From Lone_Wolf - thanks.
- TODO: use $SRCDEST - figure out what it is for and how to use it.

$ man makepkg.conf
[...]
       SRCDEST="/path/to/directory"
           If this value is not set, downloaded source files will only be stored in the current directory. Many people like to keep all source
           files in a central location for easy cleanup, so this path can be set here.
[...]

Currently, the PKGBUILD will download the file "release.1.2.5.tar.gz" which could come from anywhere, and if you have a shared downloads folder then anything else which also calls itself "release.1.2.5.tar.gz" will fool makepkg into trying to use that, and failing because of checksum mismatches (which then cause a fatal error).

Now, if you download the file in your browser, there are content-disposition headers which rename the file to something more sensible. But makepkg does not respect content-disposition headers, and in fact it cannot since then it wouldn't know where to find the downloaded file from wherever curl or wget renamed it. Thus, makepkg mandates that content-disposition will NOT be followed, and the download filename will be whatever the last url component of the initial url is.

Of course when makepkg extracts the tarball, the internal contents of the tarball contain a directory which is named "ned-release.1.2.5" regardless of how the file was downloaded.

From ugjka - thanks.
- strip not necessary - but I've added it to the install command.

Q. What would be a situation where you wouldn't want to strip?

Please remove it again, it violates Arch Linux packaging policy.

As Trilby said, the default action in makepkg is to strip all binaries for you, so it is never necessary to do so by hand. Furthermore, makepkg exposes this as a user-configurable option, for the benefit of users who wish to 1) add additional debugging $CFLAGS in order to get even more debugging symbols, 2) retain debugging symbols for the purposes of debugging, and/or 3) strip the debugging symbols into a split "ned-debug" package that can be separately installed from a personal web-hosted binary repository

From Trilby & Piri - thanks.
- Fixed depends - added gcc-libs.

$ namcap ned-1.2.5-1-x86_64.pkg.tar.xz
ned E: Dependency gcc-libs detected and not included (libraries ['usr/lib/libgcc_s.so.1'] needed in files ['usr/bin/ned'])

There is no official guidance on this. namcap is a useful tool, but is occasionally wrong, and occasionally *very* wrong -- e.g. for python software it may tell you that every single dependency is actually not needed, because it doesn't track python import statements, but it does track shared library loading.

In the specific case of gcc-libs and libc, it is rather guaranteed to be on every single Arch system without exception, no ifs ands or buts. Many people take the opinion that we don't actually need 7000 packages which depend on glibc. Many people take the opinion that you should always explicitly list this, just in case you want to install the package into a chroot that doesn't have pacman (which depends on glibc) or bash (which also depends on glibc) or coreutils (which also depends on glibc).

I am of the first opinion: don't add dependencies on glibc/gcc-libs/bash/coreutils/etc as it's impossible to have a system without them.

Obviously, I am always right. Also, I'm very humble. wink


Managing AUR repos The Right Way -- aurpublish (now a standalone tool)

Offline

#13 2019-01-21 22:12:46

Trilby
Inspector Parrot
Registered: 2011-11-29
Posts: 29,441
Website

Re: PKGBUILD review request.

Eschwartz has a completely valid point, but I respectfully disagree.  Of course, in most cases one would not list gcc-libs as it would be a dependency of something else in the depends list.  But if there really is nothing else that your package depends on *other* than gcc-libs, I say list it.  The only package that should have no dependencies at all should be a fully statically linked binary and/or just media/data files with nothing executable.

Admittedly, the point that makepkg itself is in a package that depends on gcc-libs is a particularly strong one.  But we should not exclude the possibility of a drop-in replacement for makepkg that might not necessarily depend on gcc-libs (perhaps musl, or something else entirely).

Last edited by Trilby (2019-01-21 22:15:11)


"UNIX is simple and coherent..." - Dennis Ritchie, "GNU's Not UNIX" -  Richard Stallman

Offline

#14 2019-01-22 03:21:07

nevdelap
Member
Registered: 2019-01-21
Posts: 6

Re: PKGBUILD review request.

Once again, thank you very much for the detailed feedback.

I've...

- Removed the gcc-libs dependency. (Both your reasoning makes sense, I could go either way.)
- Removed the strip.
- Added $pkgname-$pkgver.tar.gz:: to source. (That's what you meant yeah?)
- Added a .gitignore of everything.

I've not set SRCDEST. I'm not certain what to set it to, just a directory in build? I looked at a bunch of existing PKGBUILD files for the things I install in my machines with yaourt and none of them used it so I didn't find an example. But I gather adding $pkgname-$pkgver.tar.gz:: to the source= solves the problem that you described and is described in https://wiki.archlinux.org/index.php/PKGBUILD#source. Please tell me if I've got the wrong end of the stick and point me at an example package that uses it. Thanks again.

TODO: Copyrights in each source file, etc., according to the GPL How To.
TODO: Release a 1.2.6 with the copyrights.

Offline

#15 2019-01-22 03:46:00

eschwartz
Fellow
Registered: 2014-08-08
Posts: 4,097

Re: PKGBUILD review request.

nevdelap wrote:

I've not set SRCDEST. I'm not certain what to set it to, just a directory in build? I looked at a bunch of existing PKGBUILD files for the things I install in my machines with yaourt and none of them used it so I didn't find an example.

It's not meant to be set in the PKGBUILD -- it is meant to be set in the user's configuration file "makepkg.conf", if one chooses to make use of it.

To use an analogy you might be more familiar with: it is like setting $CARGO_HOME to store cached crates in some location other than $HOME/.cargo, something that is most likely *NOT* supposed to be set in one's Cargo.toml and thereby used by everyone checking out the project in order to build it.

But I gather adding $pkgname-$pkgver.tar.gz:: to the source= solves the problem that you described and is described in https://wiki.archlinux.org/index.php/PKGBUILD#source. Please tell me if I've got the wrong end of the stick and point me at an example package that uses it. Thanks again.

You are correct and this is exactly the solution I was indicating. smile

...

Some final nitpicks on your PKGBUILD:

install -d "$pkgdir/usr/bin"
install "ned-release.$pkgver/target/release/ned" "$pkgdir/usr/bin/ned"

This is unnecessary since if you take a closer look at the manpage for the install command, you can combine both into one:

install -Dm755 "ned-release.$pkgver/target/release/ned" "$pkgdir/usr/bin/ned"

The -D option allows you to install files while at the same time creating leading directories the way -d would.

The m755 is because while the install command defaults to installing files with mode 755, it is good practice to specify the exact mode you want, since you otherwise might forget to when you need mode 644 to install a data file rather than an executable.

pkgdesc="Is like grep, but with powerful replace capabilities, and more powerful than sed, as it isn't restricted to line oriented editing."

This pkgdesc is quite long, and it is recommended to stick to about 80 characters.

What about something more like "grep-like search tool with file-oriented replacement capabilities"?

Last edited by eschwartz (2019-01-22 03:50:47)


Managing AUR repos The Right Way -- aurpublish (now a standalone tool)

Offline

#16 2019-01-22 09:31:49

nevdelap
Member
Registered: 2019-01-21
Posts: 6

Re: PKGBUILD review request.

Great!

Offline

#17 2019-01-22 12:56:21

Lone_Wolf
Member
From: Netherlands, Europe
Registered: 2005-10-04
Posts: 11,866

Re: PKGBUILD review request.

If you ever need to override a makepkg option like strip that's possible.
read 'man PKGBUILD' , look for the line options (array) .

an example :
arch linux by default removes "libtool"-files with an .la extension.
Some programs require the *.la files from a dependency to compile and fail to build if they're not present .

To prevent the removal of .la files, add

options=(libtool)

That PKGBUILD will use the values set by the user in makepkg.conf except for the libtool one.


Disliking systemd intensely, but not satisfied with alternatives so focusing on taming systemd.


(A works at time B)  && (time C > time B ) ≠  (A works at time C)

Online

#18 2019-01-23 03:05:48

nevdelap
Member
Registered: 2019-01-21
Posts: 6

Re: PKGBUILD review request.

I've...

- done a new release with copyright and license notices and updated the PKGBUILD accordingly.
- Shortened the description to 80 chars.
- Replaced the two install commands with one using -D.

Thanks again.

Last edited by nevdelap (2019-01-23 03:17:10)

Offline

Board footer

Powered by FluxBB