[aur-general] TU application: Maxim Baz

Tue Nov 6 00:05:15 UTC 2018

Woohoo, reviews :) Thank you for your time Jelle!

>> - kak-lsp: de-facto official plugin that adds LSP support for kakoune editor.
> 
> - You should pass --locked to, so that the Cargo.lock file is adhered.
>   (reproducibility)

Nice one, added!

> - The package has 3 votes, the TU guidelines define that a package with
>   atleast 10 votes can be moved. Something to keep in mind, note that
>   since it benefits kakoune it's fair to add it.

Right, I was aware of this guideline at the time of writing, I added it because
kakoune's author himself names kak-lsp as the de-facto official package to use for
LSP, he confirmed that kakoune itself will never implement this, but will always
refer users to a plugin. Since kakoune is in [community], kak-lsp must be too.

>> - wire-desktop (76): End-to-end encrypted messaging app that works on Windows,
>> Linux, Mac, Android and iPhone. It is free, open-source and available
>> on Github. Although I'm co-maintaining this package on AUR, I was mostly
>> focused on contributing to the project itself: I added proper emoji support
>> (following the latest Unicode standard), emoji autocomplete and improved
>> native notifications on Linux (show user pictures, set urgency hint).
> 
> - patching should happen in prepare()
> - electron packages should use our electron package and don't download
>   it again. (I'm assuming it does btw)
> - ideally the desktop file should be from upstream or submitted upstream
> - i686 shouldn't be in the arch=() array for community packages
> - Just my personal opinion, but what the JavaScript!!!

Thank you for these, I expect making it use system electron will be the most
difficult part. Am I right to assume that if for whatever reason it turns out
to be impossible to decouple from the bundled electron version, I should keep
this package in AUR?

>> - browserpass (31): Browser extension for pass (unix password manager),
>> works in Chromium and Firefox. I became the primary project maintainer about
>> a year ago, and together with another maintainer recently started rewriting
>> it to make the architecture accommodate users' needs. I'm planning to bring
>> this to [community] after the new version is ready (we are aiming to release
>> in December). Also, someone in comments on AUR gave me a cool idea to use
>> split-packages for Chromium and Firefox browsers, I'm going to do this as
>> well (current PKGBUILD installs browserpass for both browsers, even if these
>> browsers are not installed).
> 
> No idea, what to make of the Golang stuff, melts my brains. This however
> semes to not be reproducible? Or does upstream have a lock file for
> locking it's deps?

The upstream has a lock file, and moreover the sources archive that this package
is downloading already contains all dependencies, so that PKGBUILD doesn't have to
deal with them — all it has to do is to extract the archive and run `make`.

> I'm not a fan of supporting a non-supported component i.e. goole chrome.

Will it be better in your opinion if this was a split package, providing app itself,
and then packages for google chrome, chromium and firefox? This was suggested to me
earlier, so that the package doesn't create files for browsers that are not even
installed.

I guess I could even separate them into completely different PKGBUILDs, bring
browserpass, browserpass-config-chromium and browserpass-config-firefox to community
and leave browserpass-config-google-chrome in AUR. All these browserpass-config-*
packages will provide "browserpass-config", and browserpass will depend on generic
"browserpass-config", so that user is required to install at least one.

It is vital for browserpass that configs for all browsers (where user wants to use
the app) are installed, it will not function at all without them.

> I also wonder what's with the weird location /etc/opt/chrome?

This is the predefined path that all developers of native components must respect,
it is documented in docs [1] (search for "Linux (system-wide)"). These paths are also
predefined for all other existing browsers that implement this functionality.

> You might want to use go-pie btw, to actually have PIE support
> 
> browserpass W: ELF file ('usr/bin/browserpass') lacks FULL RELRO, check LDFLAGS.
> browserpass W: ELF file ('usr/bin/browserpass') lacks PIE.

Nice, will investigate this.

>> - ttf-emojione (33): Colorful emoji font from EmojiOne. I created a Docker
>> image that is able to compile the font out of image assets, and configured a
>> Travis job for EmojiOne team so that the font is automatically being compiled
>> on every commit and attached to every Github release.
> 
> The .install scriptlet shouldn't contain documentation. 

Just to confirm, this is not as much documentation as a description of a command
that user needs to run to make this package work.

I guess I could create a wiki page for EmojiOne and put there the command to run,
but how do I let people know that they must visit the wiki? People expect to install
the font and see it working, but it won't happen until they install fontconfig file.

I've had positive experience with using .install files to inform people what else
they need to do after installation, not a single person complained in comments on AUR
about problems with font! :)

Also, just saw that wiki [4] encourages to echo important directions that are needed
to make package work, I believe my .install files fall under this category ;)

> Also, I'm not sure if we can package it, since I can't grasp the laywerspeak in
> license-free.pdf

You are right, heftig on IRC has already pointed out this to me. I contacted EmojiOne
to confirm, so far received no response. It stays in AUR until I manage to clarify this.

>> - grub-btrfs (15): Detects and includes btrfs snapshots in GRUB menu, allowing
>> to easily boot in any existing snapshot. I personally use grub-btrfs in
>> combination with snap-pac and snap-pac-grub for seamless integration with
>> snapper and pacman.
> 
> The .install scriptlet shouldn't really contain documentation. Ideally
> that should be found on the wiki or in the man pages.

Same question here, but actually worth confirming with you: is it a bad practice
to execute "systemctl daemon-reload" in post_install() function? I've seen people
do that, but so far I was refraining from executing any commands in .install files.

Creating a wiki page that would only tell people to run "systemctl daemon-reload"
after installation sounds wrong...

> Systemd units should go into /usr/lib/systemd/system not /etc, that's
> for user configuration!

Nice catch, fixed!

> Seeing you are active upstream, why doesn't it ship with a simple
> Makefile? :)

Hehe, great question, will take care of this a bit later :)

>> - python-black (10): Python code formatter that quickly gains popularity,
>> I see it being adopted more and more in the community of Python developers,
>> so I want it to be available in [community] repo.
> 
> - yay for tests being run!
> - license is installed in the wrong  directory:Missing custom license directory (usr/share/licenses/python-black)

Cannot take credit for adding test run as I'm not maintainer of this package,
but I want to move it to community as I think it's worth it. Noted your comment!

>> - gocryptfs (18): Encrypted overlay filesystem, an alternative for encfs.

Did you mean to leave a comment on gocryptfs package?

> * rebuild-detector:
> * snap-pac-grub:
> 
> - Source should not be hosted on the AUR 

I will move to Github since it simplifies collaboration, but I also want to confirm
if this is a new rule? I don't see this being mentioned on wiki [2,3,4], unless I'm blind :/

If it is a rule, let's add it to the wiki! ;)

> - Did you know lddd exists?

Yes :) The goal of this tool is different, lddd tries to be 100% correct at
a cost of running veeeeery slowly, while this package doesn't set the 100%
correctness as a goal - instead it promises to be "good enough" and _fast_. 
It takes 10 seconds on my laptop to run, and so far it was always able to notify
me about broken packages that need a rebuild. I run this tool every time I install
new updates, so that I can instantly detect if an upgrade broke some AUR packages.
I'm extremely happy with this little script :)

By the way, giving a shout-out to Robin Broda who helped me writing this tool ;)

> * i3ipc-python> * yubikey-touch-detector
> * snap-pac-grub
> 
> - Missing license, namcap warns about it.

Thanks, fixed!

> Probably missed a few things!

I appreciate your review!

Cheers,
Maxim Baz

1: https://developer.chrome.com/apps/nativeMessaging#native-messaging-host-location
2: https://wiki.archlinux.org/index.php/Arch_User_Repository
3: https://wiki.archlinux.org/index.php/Creating_packages
4: https://wiki.archlinux.org/index.php/Arch_package_guidelines

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20181106/530cb801/attachment-0001.asc>