pavucontrol: use gitlab source, clean up

[figsoda]

Motivation for this change

follow up of #142220

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[legendofmiracles]

How come the autoreconfhook needed to be added with fetchFromGitLab, but not with fetchurl?

[legendofmiracles]

Result of nixpkgs-review pr 142236 run on x86_64-linux 1

2 packages built:

• blueberry
• pavucontrol

[alyssais]

Traditionally, the package maintainer runs autoconf before generating the tarball.

[alyssais]

Why change the source?

[figsoda]

the tarball seems to have a lot more files

tarball files ignoring *.po

$ exa -TI '*.po' --group-directories-first pavucontrol-5.0
pavucontrol-5.0
├── doc
│  ├── Makefile.am
│  ├── Makefile.in
│  ├── README
│  ├── README.html
│  ├── README.html.in
│  └── style.css
├── m4
│  ├── ax_cxx_compile_stdcxx.m4
│  ├── ax_cxx_compile_stdcxx_11.m4
│  ├── intltool.m4
│  └── zp_lynx_doc.m4
├── po
│  ├── LINGUAS
│  ├── Makefile.in.in
│  ├── pavucontrol.pot
│  └── POTFILES.in
├── src
│  ├── cardwidget.cc
│  ├── cardwidget.h
│  ├── channelwidget.cc
│  ├── channelwidget.h
│  ├── devicewidget.cc
│  ├── devicewidget.h
│  ├── i18n.h
│  ├── mainwindow.cc
│  ├── mainwindow.h
│  ├── Makefile.am
│  ├── Makefile.in
│  ├── minimalstreamwidget.cc
│  ├── minimalstreamwidget.h
│  ├── pavuapplication.cc
│  ├── pavuapplication.h
│  ├── pavucontrol.cc
│  ├── pavucontrol.desktop.in
│  ├── pavucontrol.glade
│  ├── pavucontrol.h
│  ├── rolewidget.cc
│  ├── rolewidget.h
│  ├── sinkinputwidget.cc
│  ├── sinkinputwidget.h
│  ├── sinkwidget.cc
│  ├── sinkwidget.h
│  ├── sourceoutputwidget.cc
│  ├── sourceoutputwidget.h
│  ├── sourcewidget.cc
│  ├── sourcewidget.h
│  ├── streamwidget.cc
│  └── streamwidget.h
├── aclocal.m4
├── bootstrap.sh
├── compile
├── config.h.in
├── configure
├── configure.ac
├── depcomp
├── install-sh
├── intltool-extract.in
├── intltool-merge.in
├── intltool-update.in
├── LICENSE
├── Makefile.am
├── Makefile.in
├── missing
└── README

[figsoda]

Why change the source?

gitlab has a interface which makes it easier to look at the code, fetchFromGitLab is also slightly more readable than a long fetchurl

[legendofmiracles]

And it's more idiomatic, in the future we could also more easily switch to arbitrary revisions, should the need arise.

[alyssais]

gitlab has a interface which makes it easier to look at the code

You can point to this in meta.homepage. (Or, to an actual home page which presumably links to the code browser.) src is for downloading the code, not telling people where to look at it in a web browser.

And it's more idiomatic

It is not more idiomatic. For it to be idiomatic, it would have to be in line with existing practice, and nixpkgs we very frequently use tarballs when they are available.

See also #124974.

[figsoda]

You can point to this in meta.homepage

The current homepage is correct, and it includes links to the gitlab. It does have a link to the original source as a downloadable form, but there is not easy way to look at the files without downloading the tarball

[legendofmiracles]

Isn't there a case to be made for security?
The same reason why nixpkgs prefers from source over pre-compiled binaries.
It's an additional attack surface where people could include code that's not in the actual code-base; since reproducibility of the resulting tarball cannot be confirmed over different distros (read: program versions) anyways.

That's just my two cents, it seems like this is common practice so there's probably little reason to worry - and an incredibly nitpicking issue from me 😄

[Ma27]

The current homepage is correct, and it includes links to the gitlab. It does have a link to the original source as a downloadable form, but there is not easy way to look at the files without downloading the tarball

And why is this a problem? nix-shell -A pavucontrol, then unpackPhase; cd $sourceRoot; patchPhase and you're done.

Also, it's quite common for us to use release tarballs (I mean there's a reason why upstream developers prepare those), so I don't see why these are bad.

Isn't there a case to be made for security?

If we don't trust the upstream devs (i.e. the folks who maintain the source and prepare the tarballs in question), then we should stop using their code, right? ;-)

And it's more idiomatic, in the future we could also more easily switch to arbitrary revisions, should the need arise.

Sounds like premature-optimization (although I wouldn't call it optimization in this case) to me. Also - from my experience at least - we usually apply patches to e.g. incorporate important bugfixes or security fixes.

All in all, this doesn't seem useful to me, so 👎 from my side.

[figsoda]

I guess we can close this

[mohe2015]

Isn't there a case to be made for security?
The same reason why nixpkgs prefers from source over pre-compiled binaries.
It's an additional attack surface where people could include code that's not in the actual code-base; since reproducibility of the resulting tarball cannot be confirmed over different distros (read: program versions) anyways.

I personally agree with this and you don't have to suspect malicious intent of the maintainers. Maybe they have a virus on their PC or accidentally used an old vulnerable version of some build tool. Or they didn't clone the latest revision of their project with a fix. Maybe their download server was vulnerable (well you could argue this could happen with gitlab etc too but then the commit hash wouldn't match and that's easier to detect as there need to be valid commits etc (signed in the best case).)

There are literally a million additional attack vectors and for autoconf software I don't this the additional effort here is that large.

[mohe2015]

Would need an RFC though I think

[Ma27]

I personally agree with this and you don't have to suspect malicious intent of the maintainers

My point is, if we assume that an upstream developer is somehow compromised, we cannot assume that only the release tarballs are affected.

[mohe2015]

I personally agree with this and you don't have to suspect malicious intent of the maintainers

My point is, if we assume that an upstream developer is somehow compromised, we cannot assume that only the release tarballs are affected.

That is correct but I would assume that the git repositories are more closely watched and reviewed than the release artifacts.

Anyways feel free to discuss this further but I think the most useful thing would be if somebody would open an RFC about this.

[legendofmiracles]

It's also harder to verify release tarballs, because a minor version change (or different implementation) in any of the tools used to generate it will likely result in a different hash and make it impossible to compare your local release tarball with the one found upstream.

[alyssais]

It's also harder to verify release tarballs, because a minor version change (or different implementation) in any of the tools used to generate it will result in a different hash and make it impossible to compare your local release tarball with the one found upstream.

This is true, but on the other hand, hashes of git-archive(1) tarballs (used by GitHub and GitLab) can also change unexpectedly because of software updates on their end, or export-subst (a horrible git misfeature). I do think that building from Git instead of release tarballs might be a good practice to adopt, but as I've said before, we shouldn't be adopting it piecemeal and should make a concious decision to do so, with a policy that can be applied as uniform