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
Conversation
This comment has been minimized.
This comment has been minimized.
👎 as I wrote in #77507 (comment)
… The code needed to support those versions is tiny compared to the space that would be needed to keep two separate almost full-system closures (firefox is the largest package on my system) when you need to install them, so using an older nixpkgs is not a good option, IMHO.
|
Hmm, however, `firefoxPackages.firefox-esr-60` and `firefoxPackages.icecat-52` are probably okay to remove, indeed. Though I think it would be better it they vanished just before the release, not in the middle of the cycle.
|
Icecat is not about privacy, it’s about software freedom. Which the tor browser, wonderful though it is, does not do anything about it. |
Although if it’s based on an unsupported ESR, dropping it is fair enough, at least until upstream picks up. |
b0d394f
to
bb6a430
Compare
So we basically agree with removing
In terms of the other remaining To summarize: I think this PR greatly improves maintainability of the package. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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.
There was a problem hiding this 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.
@@ -355,10 +296,9 @@ stdenv.mkDerivation (rec { | |||
''; | |||
|
|||
passthru = { | |||
inherit version updateScript; | |||
inherit updateScript; | |||
version = ffversion; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
bb6a430
to
9c95a1a
Compare
9c95a1a
to
34dbc33
Compare
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.
89aba3b
to
b81c8f1
Compare
Conkeror doesn't work with any secure firefox release. Please move to some of the alternatives suggested at http://conkeror.org/Alternatives.
b81c8f1
to
9bf7d51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 eval though
# webrtcSupport breaks the aarch64 build on version >= 60, fixed in 63. | ||
# https://bugzilla.mozilla.org/show_bug.cgi?id=1434589 |
There was a problem hiding this comment.
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.
firefoxPackages.*: remove unsupported packages, clean up derivation (cherry picked from commit 1b2b9da)
This PR is now a perfect example of what "an abuse of commit rights" means.
Anyways, SLNOS delivers an indefinitely maintained revert + tor-browser revival as promised: https://github.com/SLNOS/nixpkgs/commits/it-aint-broken/master/20200210 (which also reverts similarly utterly senseless changes to display managers as a bonus) and a corresponding overview issue: SLNOS#1 in case you think I missed something else not broken and useful that was recently removed (which I'm sure I did).
Ta-da! Pachi-pachi-pachi for the first public SLNOS branch, I guess.
|
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.
That's another occurence of disrespectful language, but I'll give up elaborating on this. |
> 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.
As far as I can see, there are at least as many people disagreeing with this as agreeing:
- explicitly directly against are me and @7c6f434c,
- against as a consequence of #77507 are @joachifm, and, I assume, @vcunat and @ahiaao,
- also, maybe, @dtzWill, judging from the reaction to the SLNOS branch.
Good luck with maintaining that branch.
Thanks. But if this is meant to be sarcasm, then let me remind you that SLNOS was made in 2017, ATM it has >500 patches on top of Nixpkgs, a lot of those are reverts, and somehow we manage to maintain them without much fuss. In practice, Nixpkgs, and especially NixOS, just doesn't _change_ all that much (outside of some people removing old useful stuff zealously without technical reasons for it).
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
Ha-ha, again. Above I meticulously explained (several times) that your method of "lessening the burden" just could not work if we are to have those packages. You can't have an expression that supports all those build options without actually maintaining them. This is not Equestria, this is real life.
and in the case of tor-browser, doesn't have questionable fingerprinting counter-measurements.
More FUD, again.
disrespectful
At least now it is not "violent" now. What adjective would you use for the next level of definition inlining, I wonder.
The argument for these changes "is based on demonstrably incorrect technical reasoning and contradictory requirements" => "makes absolutely no sense" => "is utterly senseless" ("disrespectful") => "is [CENSORED]" ("violent!")
The meaning does not change.
|
On 20:26 02.03.20, volth wrote:
So now `i686-linux` has no Firefox at all, right?
Before going back to picking an ancient version of version why not use
the `-bin` packages? That should be available for i686-linux.
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.
|
We currently don't really fully support running NixOS on i686 or armv7l, and even Mozilla has problems building it on 32bit, so I wouldn't call this a major regression.
If you really want to have Firefox on 32bit in nixpkgs, fix it with the current version, or use a binary version.
If you want an older version, import from an older nixpkgs, but we don't add back unsupported and insecure software. Nixpkgs is not a dumpster for broken and unmaintained packages.
|
@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. 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. |
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 |
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 |
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
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)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 :-)