Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

firefoxPackages.*: remove unsupported packages, clean up derivation #79115

Merged
merged 7 commits into from Feb 9, 2020

Conversation

@flokli
Copy link
Contributor

@flokli flokli commented Feb 2, 2020

Follow-up of the discussion in #77452.

This removes browsers from the firefoxPackages.* attrset that have open security issues and/or are unmaintained.

Keeping logic to build these packages greatly increases complexity of the firefox derivation, which makes maintenance much harder than necessary. This keeps support for the current firefox release and the ESR variant.

firefoxPackages.firefox-esr-52

There's a comment next to it explaining some reasons on why this might be needed (plugins never ported to WebExtensions API).
If sb. needs to access these, fetching that version of firefox from a old nixpkgs checkout, is probably a better solution than to keep all the complexity.

firefoxPackages.firefox-esr-60

I don't really understand the reasons for that. It's not supporting WebExtensions anymore, and it's an old ESR release. Probably people are just fine taking the current ESR release.

firefoxPackages.icecat

Icecat upstream seems to be maintained by a single person, the current version has some open security issues (for months now). If people care about privacy, it's probably a much saner choice to go with the Tor Browser or a recent Firefox with some privacy plugins enabled.

firefoxPackages.icecat-52

I don't see any reason in keeping this. firefoxPackages.firefox-esr-52, while terribly out of date, was meant for offline use only, so why is there a more privacy-focused variant of that for these offline applications?

Motivation for this change

Increase maintainability of the firefox package.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @oxij

Note that I didn't yet built any of these, and I could have made some errors while merging all these conditionals. I'd appreciate everybody taking a very close look :-)

@flokli

This comment was marked as off-topic.

@oxij
Copy link
Contributor

@oxij oxij commented Feb 3, 2020

@oxij
Copy link
Contributor

@oxij oxij commented Feb 3, 2020

@alyssais
Copy link
Member

@alyssais alyssais commented Feb 3, 2020

If people care about privacy, it's probably a much saner choice to go with the Tor Browser or a recent Firefox with some privacy plugins enabled.

Icecat is not about privacy, it’s about software freedom. Which the tor browser, wonderful though it is, does not do anything about it.

@alyssais
Copy link
Member

@alyssais alyssais commented Feb 3, 2020

Although if it’s based on an unsupported ESR, dropping it is fair enough, at least until upstream picks up.

@flokli flokli force-pushed the flokli:simplify-firefox branch from b0d394f to bb6a430 Feb 4, 2020
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 4, 2020

So we basically agree with removing

  • firefoxPackages.firefox-esr-60
  • firefoxPackages.icecat-52
  • firefoxPackages.icecat (possibly reintroduce once the upstream situation has improved)

In terms of the other remaining -52 variant, I refer to my argument at #77507 (comment)

To summarize: I think this PR greatly improves maintainability of the package.

@flokli flokli marked this pull request as ready for review Feb 4, 2020
@oxij

This comment was marked as off-topic.

@grahamc
grahamc approved these changes Feb 5, 2020
Copy link
Member

@grahamc grahamc left a comment

LGTM once it passes ofborg. If icecat gets an active maintainer, let's put it back. The older versions don't belong in nixpkgs.

Copy link
Member

@andir andir left a comment

Just a quick pass. Still have to build, test and read through it more closely.

@@ -355,10 +296,9 @@ stdenv.mkDerivation (rec {
'';

passthru = {
inherit version updateScript;
inherit updateScript;
version = ffversion;

This comment has been minimized.

@andir

andir Feb 5, 2020
Member

I am tempted to propose changing ffversion back to version. I was never really happy about that change but it made sense when we shared the expression with other browser flavors. Opinions?

This comment has been minimized.

@flokli

flokli Feb 6, 2020
Author Contributor

done in an additional commit.

This comment has been minimized.

@andir

andir Feb 9, 2020
Member

Clearly @oxij has opinions about this point. I am fine with leaving it in here. I asked for opinions for a reason.

Maybe we should indeed revert this, if only, to show good intend and not cause further harm. After all it doesn't really mean much to me (at least).

Copy link
Member

@worldofpeace worldofpeace left a comment

Copy link
Contributor

@oxij oxij left a comment

As it is, the last commit assumes that #77507 will never get merged and icecat will never get updated.

@flokli flokli force-pushed the flokli:simplify-firefox branch from bb6a430 to 9c95a1a Feb 6, 2020
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 6, 2020

@andir, I adressed all the requested changes (including the change to version.

@oxij I'll address your remarks in #77507 itself.

@flokli flokli force-pushed the flokli:simplify-firefox branch from 9c95a1a to 34dbc33 Feb 6, 2020
@flokli flokli requested a review from worldofpeace Feb 6, 2020
pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
Conkeror doesn't work with any secure firefox release.
Please move to some of the alternatives suggested at
http://conkeror.org/Alternatives.
@flokli flokli force-pushed the flokli:simplify-firefox branch from b81c8f1 to 9bf7d51 Feb 9, 2020
Copy link
Member

@worldofpeace worldofpeace left a comment

👍 eval though

@andir
andir approved these changes Feb 9, 2020
@worldofpeace worldofpeace added this to the 20.03 milestone Feb 9, 2020
@ofborg ofborg bot requested a review from andir Feb 9, 2020
@worldofpeace worldofpeace merged commit 1b2b9da into NixOS:master Feb 9, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
# webrtcSupport breaks the aarch64 build on version >= 60, fixed in 63.
# https://bugzilla.mozilla.org/show_bug.cgi?id=1434589
Comment on lines 33 to 34

This comment has been minimized.

@flokli

flokli Feb 9, 2020
Author Contributor

that comment can go away, too, as we don't have that version in here anymore.

@flokli flokli deleted the flokli:simplify-firefox branch Feb 9, 2020
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 9, 2020
firefoxPackages.*: remove unsupported packages, clean up derivation

(cherry picked from commit 1b2b9da)
oxij added a commit to SLNOS/nixpkgs that referenced this pull request Feb 10, 2020
…ers"

This reverts commit 1b2b9da, reversing
changes made to 2fc3322.
@oxij
Copy link
Contributor

@oxij oxij commented Feb 10, 2020

@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 10, 2020

This PR is now a perfect example of what "an abuse of commit rights" means.

Seems we disagree on that statement. This has been reviewed by multiple people in the community, and the majority seems to have agreed on it.

Good luck with maintaining that branch. As explained in the other PR, we're still happy to reintroduce a well-maintained patchset introducing something more software-freedom-friendly icecat-like, or a NixOS-optimized Tor Browser, if it doesn't cause too much maintenance burden to the regular firefox package, and in the case of tor-browser, doesn't have questionable fingerprinting counter-measurements.

similarly utterly senseless changes to […]

That's another occurence of disrespectful language, but I'll give up elaborating on this.

@oxij
Copy link
Contributor

@oxij oxij commented Feb 10, 2020

@volth
Copy link
Contributor

@volth volth commented Mar 3, 2020

So now i686-linux has no Firefox at all, right?

@volth
Copy link
Contributor

@volth volth commented Mar 3, 2020

on 32-bit platforms, building of firefox-esr-68 and firefox fails with

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
error: could not compile `gkrust`.

I'd consider restore firefox-esr-52 and firefox-esr-60 until our cross-compilation subsystem be able to compile 32-bit targets using 64-bit compilers.

Yes, aarch32-linux and i686-linux are "not officially supported", but their support quickly slides towards "unusable" and "decayed". Absence on a web-browser in NixOS 20.03 is blocker which prevents upgrading from 19.09.

cc @Ericson2314 @matthewbauer

@andir
Copy link
Member

@andir andir commented Mar 3, 2020

@volth
Copy link
Contributor

@volth volth commented Mar 3, 2020

Per our defintion that is currently unfree. The other one would be "insecure". Choose your pain. I'd probably take a bit of unfree-ish software over insecure.

If you really interested in my opinion: I do not care neither about "(un)free" nor about "(in)secure" (and agree with @oxij that reducing the discourse to such a fake dichotomy is merely marketing FUD).
I only do care about a convenience to apply my patches to customize software that way, it is why nixpkgs is chosen over binary distros. So your advice to use -bin is like an advice to switch to, say, Manjaro, because 32-bit NixOS is failed.

@volth
Copy link
Contributor

@volth volth commented Mar 3, 2020

Of course if would be better to find a way to build firefox 73 for 32-bit platforms.
But until it is done, removing the latest (while at the same time ancient) versions available for 32-bit platforms is a destructive change

@flokli
Copy link
Contributor Author

@flokli flokli commented Mar 3, 2020

@volth
Copy link
Contributor

@volth volth commented Mar 3, 2020

@Flocki: that literally means "stay with 19.09 on 32 platforms".
Importing a derivation from 19.09 on 20.03+ is very fragile, because GUI apps are not well isolated, there are system-wide GTK env vars, something in /run/current-system/sw/lib etc (I have not tried it with firefox yet, but tried chromium from older versions of nixpkgs. It basically does not work at all) Many versions of curl do work OK, but many versions of GUI apps... each one with own GTK version... no. GUI app have to be build against 20.03 libraries which leak to system-wide global state. So the only feasible option is to restore firefox-esr-52 in a nixpkgs fork.

@flokli
Copy link
Contributor Author

@flokli flokli commented Mar 3, 2020

@volth I agree GUI applications sometimes aren't well-isolated, and may behave weirdly. I didn't want to say importing from older versions of nixpkgs is beautiful solution here - but there's ways to workaround the problem locally, without shifting maintenance burden to the nixpkgs maintainers.
A probably less hacky way could be to distribute an overlay containing the older firefox build recipe, if there's somebody maintaining that.

I'm also quite unhappy about the fact building 32 bit firefox doesn't work well currently - but simply re-adding older versions (and ignoring all the security and maintenance nightmares that come with it) doesn't fix the real problem.

If we want to make 32 bit firefox happen, we should get it to work on current versions, and talk with upstream to get it better integrated.

@danbst
Copy link
Contributor

@danbst danbst commented Apr 17, 2020

firefox from older checkout may stop working on newer machines. I've experienced it at least once, due to some GLIBC mismatch. Which is understandable, even if we fix reproducibility issues, we don't backport those to older releases, so older releases still have impurities. Or just glibc fails to provide backward compatibility.

Given I'm a user of ESR 52 + Icedtea (previously + Oracle JRE, but luckily Icedtea has matured to cover my usecase), I'd like to have this as a prebuilt setup. I'm fine if this will be firefox-esr-52-bin.

@volth
Copy link
Contributor

@volth volth commented Apr 17, 2020

I've experienced it at least once, due to some GLIBC mismatch

It needs a small patch for glibc 2.30
You can pick it volth@a8ac169 on in @oxij repo.
Or given that there are already few firefox-52 users (and with releasing of 20.03 more people will realize that firefox-52 is gone and broken), we might try to reintroduce it back to nixpkgs' master instead of cherry-picking repos of each other

@flokli
Copy link
Contributor Author

@flokli flokli commented Apr 19, 2020

According to #79115 (comment), SLNOS delivers an indefinitely maintained revert of this PR, and what they call tor-browser.

I don't think it should be reintroduced to nixpkgs.

Introducing a firefox-esr-52-bin could be up for discussion IMHO, if it's marked as insecure and users are much made aware of the implications of using such an abandoned version of a browser.

@volth
Copy link
Contributor

@volth volth commented Apr 19, 2020

According to #79115 (comment), SLNOS delivers an indefinitely maintained revert of this PR, and what they call tor-browser.

Could you please point to nixpgks release-20.03 and master with this PR reverted?
The last revert I saw was before nixpkgs upgraded glibc to 2.30, which broke old firefoxes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.