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 has been minimized.

@oxij
Copy link
Member

oxij commented Feb 3, 2020 via email

@oxij
Copy link
Member

oxij commented Feb 3, 2020 via email

@alyssais
Copy link
Member

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 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
Copy link
Contributor Author

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 February 4, 2020 22:23
@oxij

This comment has been minimized.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkgs/applications/networking/browsers/firefox/common.nix Outdated Show resolved Hide resolved
@@ -355,10 +296,9 @@ stdenv.mkDerivation (rec {
'';

passthru = {
inherit version updateScript;
inherit updateScript;
version = ffversion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in an additional commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@oxij oxij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flokli
Copy link
Contributor Author

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.

There's not really a reason to ship an unsupported ESR variant of
firefox, and if one really needs it, it's also possible to just checkout
an older version of nixpkgs.
firefoxPackages.icecat was removed as even its latest upstream version
is based on an unsupported ESR release with open security issues.
firefoxPackages.firefox-esr-52 was removed as it's an unsupported ESR
with open security issues. If you need it because you need to run some
plugins not having been ported to WebExtensions API, import it from an
older nixpkgs checkout still containing it.
with firefox 64 being the latest version, and the removal of
"tor-browser/icecat-like" variants, we can greatly simplify the common
firefox derivation.
Conkeror doesn't work with any secure firefox release.
Please move to some of the alternatives suggested at
http://conkeror.org/Alternatives.
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 eval though

@worldofpeace worldofpeace added this to the 20.03 milestone Feb 9, 2020
@ofborg ofborg bot requested a review from andir February 9, 2020 22:48
@worldofpeace worldofpeace merged commit 1b2b9da into NixOS:master Feb 9, 2020
Comment on lines 33 to 34
# webrtcSupport breaks the aarch64 build on version >= 60, fixed in 63.
# https://bugzilla.mozilla.org/show_bug.cgi?id=1434589
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flokli flokli deleted the simplify-firefox branch February 9, 2020 23:02
dtzWill pushed 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
Member

oxij commented Feb 10, 2020 via email

@flokli
Copy link
Contributor Author

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
Member

oxij commented Feb 10, 2020 via email

@andir
Copy link
Member

andir commented Mar 3, 2020 via email

@flokli
Copy link
Contributor Author

flokli commented Mar 3, 2020 via email

@flokli
Copy link
Contributor Author

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 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.

@flokli
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

None yet

8 participants