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

ungoogled-chromium: init at 81.0.4044.92-2 #76082

Merged
merged 1 commit into from Apr 24, 2020
Merged

Conversation

@squalus
Copy link
Contributor

@squalus squalus commented Dec 20, 2019

Motivation for this change

Adds support for the ungoogled-chromium patch set to the nixpkgs chromium derivation. Requested in #53579.

ungoogled-chromium is Google Chromium, sans dependency on Google web services. It also features some tweaks to enhance privacy, control, and transparency (almost all of which require manual activation or enabling). ungoogled-chromium retains the default Chromium experience as closely as possible.

  • Applies the full patch set (except for Widevine removal, which is covered by an existing patch)
  • Runs all ungoogled-chromium patching utilities, including binary pruning and domain substitution.
  • Applies the GN flags from ungoogled-chromium. Unforunately they are duplicated in the chromium nix expression. An alternative would be to parse them out of the flags.gn file in the ungoogled-chromium repo. That would still involve some tweaking and deduping though. I'm interested in feedback as to whether it's worth doing this.
  • Supports multiple versions of chromium with the ungoogled-src.nix file, which maps a chromium version to a ungoogled-chromium revision (although it's not able to be auto-updated like upstream-info.nix).
  • Tested 79.0.3945.79 on NixOS 19.09. Everything works ok for me, including WebGL acceleration.

The patch set can be enabled with the following nixpkgs config:

chromium = {
     ungoogled = true;
};

I'm very interested in feedback!

Prior work: #51195.

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 (Gentoo)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @bendlas @ivan @thefloweringash

, patch
}:
{ rev, sha256 }:
stdenv.mkDerivation {

This comment has been minimized.

@worldofpeace

worldofpeace Dec 20, 2019
Member

This expression lacks good whitespace like other expressions in nixpkgs.

See like https://github.com/NixOS/nixpkgs/blob/master/pkgs/desktops/pantheon/apps/elementary-camera/default.nix

This comment has been minimized.

@squalus

squalus Dec 20, 2019
Author Contributor

Updated whitespace.

@squalus squalus force-pushed the squalus:ungoogled branch from b72bb2e to 645beb6 Dec 20, 2019
Copy link
Member

@ivan ivan left a comment

If you would like to package ungoogled-chromium, I think it should be its own package, with the upstream versions decoupled from chromium's versions. Chromium is already frequently difficult and time-consuming (as in, whole day down the drain) to update. I cannot hold back a new release with 50 CVE fixes just because the ungoogled-chromium patchset has not been updated yet. This would also burden the chromium maintainer with another 3+ hour build and smoke test.

ungoogled-chromium is basically a separate browser product, even if it is distributed as a patchset, so I recommend copying nixpkgs chromium/ into a new directory and replacing the maintainers :-)

@squalus
Copy link
Contributor Author

@squalus squalus commented Dec 20, 2019

I'd be willing to copy/paste into a new package. I agree that the chromium release should not be held back due to a delay in the patchset release.

@squalus squalus force-pushed the squalus:ungoogled branch 2 times, most recently from 78822b4 to ef5e9ce Jan 3, 2020
@squalus
Copy link
Contributor Author

@squalus squalus commented Jan 3, 2020

I updated the PR to duplicate the chromium expressions.

The maintenance workflow will be to start with copy/pasted chromium expressions, then reapply the ungoogled-chromium expression diff on top of that, then update the ungoogled-chromium sources as needed.

Ideally I wouldn't need to modify the copy/pasted chromium expressions at all, instead just naively duplicate them and keep the patchset-related stuff in separate files. However, I couldn't figure out any way to do that with the current chromium expressions. What I would need is the ability to externally (through an override maybe) provide extra gnFlags and add extra postPatch steps.

@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Jan 14, 2020

@squalus Any update on your work on a separate package? I'd love to have Ungoogled chromium available for NixOS.

@squalus
Copy link
Contributor Author

@squalus squalus commented Jan 15, 2020

@LouisDK1 Yeah, in the latest update to this PR there's a separate chromiumUngoogled package available. I've been running it for about a week.

@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Jan 16, 2020

@LouisDK1 Yeah, in the latest update to this PR there's a separate chromiumUngoogled package available. I've been running it for about a week.

Awesome. I differently think this should get into the main repo.
Wouldn't it be a good idea to add "enableParallelBuilding = true;" somewhere in common.nix to minimize build time?
EDIT 2: Parallel building already enabled using "ninja".

@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Jan 22, 2020

@squalus - Please update package to: 79.0.3945.130-2 and if possible change the headline to match the fact that this is a separate package.

@squalus squalus changed the title chromium: add support for ungoogled patches chromiumUngoogled: init at 79.0.3945.88-1 Jan 23, 2020
@squalus
Copy link
Contributor Author

@squalus squalus commented Jan 23, 2020

@LouisDK1 Updated the title, and left the version as-is. For now I'm aiming for the smallest possible diff from the "upstream" chromium nix expressions. I'll leave the testing and stabilization of new versions up to them.

@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Jan 25, 2020

@ivan He has done it. Could you review and maybe accept the package? :)

Copy link
Member

@worldofpeace worldofpeace left a comment

Can we not call it chromiumUngoogled it's really backwards?
ungoogled-chromium makes more sense https://repology.org/project/ungoogled-chromium/versions.
(and please rename the directories)

@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Jan 26, 2020

@squalus ping

@squalus squalus force-pushed the squalus:ungoogled branch from ef5e9ce to f503f65 Jan 28, 2020
@squalus squalus changed the title chromiumUngoogled: init at 79.0.3945.88-1 ungoogled-chromium: init at 79.0.3945.117-1 Jan 28, 2020
@squalus
Copy link
Contributor Author

@squalus squalus commented Jan 28, 2020

  • changed package name and directories to ungoogled-chromium
  • updated version to 79.0.3945.117-1
@squalus squalus force-pushed the squalus:ungoogled branch from f503f65 to 98c2001 Jan 28, 2020
Copy link
Member

@worldofpeace worldofpeace left a comment

I've built ungoogled-chromium and it works

@squalus squalus force-pushed the squalus:ungoogled branch from 98c2001 to 065aa45 Jan 31, 2020
@squalus squalus changed the title ungoogled-chromium: init at 79.0.3945.117-1 ungoogled-chromium: init at 79.0.3945.130-2 Jan 31, 2020
@squalus squalus force-pushed the squalus:ungoogled branch from 065aa45 to 604a7a3 Jan 31, 2020
@rycee
Copy link
Member

@rycee rycee commented Feb 5, 2020

@worldofpeace Do you feel that the changes you requested were sorted? Your review is still marked as requesting changes 🙂

@wizeman
Copy link
Member

@wizeman wizeman commented Feb 5, 2020

Works for me 👍 Thanks!

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

@rycee rycee left a comment

LGTM

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 5, 2020

@worldofpeace Do you feel that the changes you requested were sorted? Your review is still marked as requesting changes slightly_smiling_face

I don't feel I could properly commentate on adding another chromium flavor in nixpkgs other than it works. I don't completely approve of the maintainability of completely duplicating the chromium expressions. But I would be happy to use this.

@rycee
Copy link
Member

@rycee rycee commented Feb 5, 2020

@worldofpeace Yeah, agreed. It would be great if some more of the common parts could be abstracted out of these packages. I'm inclined to merge as-is, though, since I suppose that would be a job too large for this PR.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 5, 2020

@worldofpeace Yeah, agreed. It would be great if some more of the common parts could be abstracted out of these packages. I'm inclined to merge as-is, though, since I suppose that would be a job too large for this PR.

It was actually suggested #76082 (review).
I have no disagreements if you merge, I just hope to not see the expressions decay.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 7, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/28

*/
tryFetch = url: let
# SHA1 hash collisions from https://shattered.io/static/shattered.pdf:
collisions = runCommand "sha1-collisions" {

This comment has been minimized.

@volth

volth Feb 10, 2020
Contributor

That April 1's collision-based update has been removed from nixpkgs, hasn't it?

This comment has been minimized.

@squalus

squalus Feb 16, 2020
Author Contributor

It's still here:

(this PR is just a copy of the chromium directory with whatever small changes are required to apply the ungoogled-chromium patchset)

@squalus squalus force-pushed the squalus:ungoogled branch from 604a7a3 to bd7c9e5 Feb 16, 2020
@squalus squalus changed the title ungoogled-chromium: init at 79.0.3945.130-2 ungoogled-chromium: init at 80.0.3987.106-1 Feb 16, 2020
@squalus squalus force-pushed the squalus:ungoogled branch from bd7c9e5 to ae2a0a4 Feb 17, 2020
@LouisDK1
Copy link
Contributor

@LouisDK1 LouisDK1 commented Feb 23, 2020

@worldofpeace - can you "accept" your request as it has been fulfilled?
Anything else holding back this package from being merged?

@travankor
Copy link

@travankor travankor commented Mar 8, 2020

:shipit:

@primeos primeos mentioned this pull request Mar 12, 2020
2 of 2 tasks complete
@squalus squalus force-pushed the squalus:ungoogled branch from ae2a0a4 to 00fd934 Mar 24, 2020
@symphorien
Copy link
Contributor

@symphorien symphorien commented Mar 26, 2020

I compiled this PR successfully.

@squalus squalus force-pushed the squalus:ungoogled branch from 00fd934 to 86f1b5a Apr 11, 2020
@squalus squalus changed the title ungoogled-chromium: init at 80.0.3987.106-1 ungoogled-chromium: init at 80.0.3987.163-1 Apr 11, 2020
@squalus squalus force-pushed the squalus:ungoogled branch from 86f1b5a to 7205bd6 Apr 13, 2020
@squalus squalus changed the title ungoogled-chromium: init at 80.0.3987.163-1 ungoogled-chromium: init at 81.0.4044.92-2 Apr 13, 2020
@neirenoir
Copy link

@neirenoir neirenoir commented Apr 23, 2020

@worldofpeace the changes you requested were fixed. Could we get the patch merged, pretty please? :(

@worldofpeace worldofpeace merged commit b4d7725 into NixOS:master Apr 24, 2020
16 checks passed
16 checks passed
ungoogled-chromium, ungoogled-chromium.passthru.tests on aarch64-linux Timed out, unknown build status
Details
ungoogled-chromium, ungoogled-chromium.passthru.tests on x86_64-linux Timed out, unknown build status
Details
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="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7205bd6"; rev="7205bd64a38d73ff8c46a5bb9d4732da1a26ac02"; } ./pkgs/t
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
@primeos
Copy link
Member

@primeos primeos commented Apr 24, 2020

My delayed opinion (disclaimer: probably not really constructive, feel free to skip)


tbh I'm not sure if #76082 (review) was a good idea (cop-pasting the Chromium expression instead of overriding it) because:

  • I consider it a bad practice in general
  • It might be more difficult to track the differences
    • But maybe not since we have good diff tooling :)
  • Both expressions are likely to diverge over time if we aren't careful, resulting in unnecessary/unintended functional differences
  • If ungoogled-chromium isn't actively maintained, I'll quickly fall behind and get insecure
    • E.g. currently it's already one patch release behind

But at least it doesn't make maintaining chromium more difficult than it already is :)

But I'm too late with this comment (sorry about this delayed note) and who knows, it might be better the way it is anyway.


@squalus I'd suggest the following to maintain this (I'm the current chromium maintainer):

  • Regularly check for differences, e.g. with:
    git diff --no-index pkgs/applications/networking/browsers/{,ungoogled-}chromium
    • E.g. currently there are VA-API, flashplayer-ppapi, and version differences
    • Re-using changes from chromium will likely help you avoid build failures (I generally try to fix them in advance via testing the beta and dev channels as well)
  • I can ping/notify you on chromium PRs if you want
  • If possible ping/notify me about changes that make sense for chromium as well so I can re-use them
    • I.e. we'll try to "cherry-pick" changes in both directions
  • Maybe find a second maintainer, e.g. for NixOS stable (backports) and as fallback

Also: Thanks for adding this package and good luck! I'm looking forward to a successful cooperation :)

@squalus
Copy link
Contributor Author

@squalus squalus commented Apr 28, 2020

Thanks for merging!

@primeos thanks for the comments. I have some maintenance scripts that will do this process semi-automatically. I'll commit them if I can make them look sensible. My plan for maintenance is just to periodically take the entire chromium directory, blindly copy it over, and apply the small set of ungoogled diffs. I would also like to work on the chromium expressions to make this process easier. 👍

No need to notify me over changes, I'll be periodically monitoring them (but feel free). I'm also looking forward to working with you!

@squalus squalus deleted the squalus:ungoogled branch Apr 28, 2020
@danielfullmer
Copy link
Contributor

@danielfullmer danielfullmer commented Apr 28, 2020

@squalus Also, I've been testing the chromium PRs by @primeos in the past. If interested, you can CC me on future update PRs of ungoogled-chromium and I can do similar basic building / testing. I have a beefy 32-core threadripper which makes this relatively quick for me.

@squalus
Copy link
Contributor Author

@squalus squalus commented Apr 28, 2020

@danielfullmer Will do. Thank you!

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

You can’t perform that action at this time.