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

Electron updates #271677

Merged
merged 7 commits into from
Dec 6, 2023
Merged

Electron updates #271677

merged 7 commits into from
Dec 6, 2023

Conversation

yu-re-ka
Copy link
Contributor

@yu-re-ka yu-re-ka commented Dec 2, 2023

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Priorities

Add a 👍 reaction to pull requests you find important.

@yu-re-ka yu-re-ka requested a review from Janik-Haag December 4, 2023 09:28
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Dec 4, 2023
@yu-re-ka yu-re-ka requested a review from yayayayaka December 4, 2023 09:28
Copy link
Member

@Janik-Haag Janik-Haag left a comment

Choose a reason for hiding this comment

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

LGTM. I just built teams-for-linux which uses electron and works fine (only took 983 mins)

@delroth delroth added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 5, 2023
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 5, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 5, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 5, 2023
@@ -18488,9 +18488,10 @@ with pkgs;
electron_22 = electron_22-bin;
electron_23 = electron_23-bin;
electron_24 = electron_24-bin;
electron_25 = if lib.meta.availableOn stdenv.hostPlatform electron-source.electron_25 then electron-source.electron_25 else electron_25-bin;
electron_25 = electron_25-bin;
Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 6, 2023

Choose a reason for hiding this comment

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

Can we please keep this? Lots of things are still using electron 25 like bitwarden and I want to stay as blob free as possible.

Since we already have the patches and all the code it is currently no extra maintenance and if things break please give me a ping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would not be built by hydra anyways, since it is EOL and thus marked as insecure

Copy link
Member

Choose a reason for hiding this comment

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

According to https://endoflife.date/electron Electron 25 won't be EOL before January 2.

Copy link
Member

Choose a reason for hiding this comment

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

Not according to their documentation.

Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 7, 2023

Choose a reason for hiding this comment

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

Then they changed this again. I've just recently updated all the dates in endoflife.date with the information available. endoflife-date/endoflife.date@a274819

It would not be built by hydra anyways, since it is EOL and thus marked as insecure

This is garbage. We must stop doing this. Just because something is EOL we cannot gate all the people not using it and requiring them to buy very good PCs.

This also breaks bitwarden desktop client. The ecosystem is just not as fast as electron updates and EOLs their software and there is not much we can do about this if we want to be able to use electron software.

Copy link
Member

Choose a reason for hiding this comment

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

Please see #272658

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 6, 2023
Copy link
Member

@emilylange emilylange left a comment

Choose a reason for hiding this comment

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

The chromium bits look good.
Thanks a lot :)

Copy link
Member

Choose a reason for hiding this comment

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

This patch would have been more fitting in the ./patches directory, but given https://github.com/NixOS/nixpkgs/blob/a7153d8843a2b087f69575a93653155c592b71ca/pkgs/applications/networking/browsers/chromium/cross-compile.patch also isn't in there, I will probably just move those two in some PR in the future.

(I will probably redo #264326 soon-ish)

@emilylange
Copy link
Member

emilylange commented Dec 6, 2023

PS: @yu-re-ka, would you be interested in some sort of chromium/electron coordination chat on Matrix?
Yes, we have #213862, but I feel something closer to an actual chat room might be more fitting.

cc @networkException (all the other [edit: chromium] maintainers have been inactive for months and should probably get removed from meta.maintainers)

@networkException
Copy link
Member

Sure, yes

@yu-re-ka
Copy link
Contributor Author

yu-re-ka commented Dec 6, 2023

I'm not opposed to being added to such a chat, but I'm not a chromium maintainer. I'm a firefox user and only use the chromium expression through electron (element-desktop, specifically).

@emilylange
Copy link
Member

That's perfectly fine.

I just thought of some sort of coordination chat because of how dependent electron is on the chromium derivation.

It honestly slightly unsettles me how much but oh well.
It makes sense, don't get me wrong.

But it does have a couple of downsides from chromium's perspective.

Fun fact: I am technically a chromium maintainer, but don't use chromium (nor ungoogled-chromium) either 🫠


Are you fine with me merging this PR, or is there anything left blocking?

@yu-re-ka
Copy link
Contributor Author

yu-re-ka commented Dec 6, 2023

I'm fine with merging

@emilylange emilylange merged commit 04236bd into NixOS:master Dec 6, 2023
22 of 23 checks passed
Copy link
Contributor

github-actions bot commented Dec 6, 2023

Successfully created backport PR for release-23.11:

@RaitoBezarius
Copy link
Member

I am a pseudo chromium maintainer and am using chromium alas, not a lot of time available, but I do monitor what is going on and my silence is tacit approval, so feel free to poke me if you really need a chromium user perspective. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants