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

Revert "electron_25: eol" #272658

Closed

Conversation

SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 7, 2023

This reverts commit 9652f98.

Description of changes

Currently testing locally. This revives the electron desktop application. @dotlambda

Also see this thread #271677 (comment)

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.

Add a 👍 reaction to pull requests you find important.

This reverts commit 9652f98.
@SuperSandro2000 SuperSandro2000 mentioned this pull request Dec 7, 2023
13 tasks
Copy link
Contributor

@yu-re-ka yu-re-ka left a comment

Choose a reason for hiding this comment

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

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.

You are entirely ignoring that we did not remove electron-bin 25.x from the tree. The user can simply opt in to using the insecure electron 25 bin package using nixpkgs.config.permittedInsecurePackages, which the evalution failure message will spit out a snippet for you on how to do that.

Besides that, it should be a motivation to interact with bitwarden upstream to get their electron dependency updated.

When we added electron built from source, it was already controversial if we should add so many expensive hydra jobs, and then we got the green light based on only having the supported (meaning the last 3) versions built in hydra. We can not build 6+ chromium derivations for each staging cycle, especially on aarch64-linux this will kill hydra and even worse backfire into not being able to have Electron built from source at all.

@SuperSandro2000
Copy link
Member Author

Besides that, it should be a motivation to interact with bitwarden upstream to get their electron dependency updated.

The Electron 26 update was reverted because it caused an issue bitwarden/clients#6560

I've asked if there is some progress we can follow bitwarden/clients#6573 (comment)

We can not build 6+ chromium derivations for each staging cycle, especially on aarch64-linux this will kill hydra and even worse backfire into not being able to have Electron built from source at all.

but since then we removed the chromium dev/beta variants. Can't hydra hold this extra package for a month longer? Having to rely on binary packages because of that is not a resolution I want to accept.

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Dec 7, 2023

Thank you for looking into the upstream issues.

but since then we removed the chromium dev/beta variants. Can't hydra hold this extra package for a month longer?

The chromium dev/beta variants were never built by hydra, so removing these variants does not reduce load on hydra.

If you can get a confirmation from one of the people who are able to judge the Hydra capacity, and with the statement that the intention is not to keep around old electron releases around forever, I will retract my veto from adding back the source-built derivation for electron_25, however I still think using the electron 25 package (both bin and source) should throw at least an eval warning at users.

Having to rely on binary packages because of that is not a resolution I want to accept.

I'm totally on the same side here as not wanting to have binary packages on my system was the original motivation for packaging electron from source. I just disagree on how this should be achieved in this specific case. In the best case, a NixOS+bitwarden user would use the ability to build Electron with their own patchsets to contribute a fix for the underlying issue, and make it possible to run bitwarden using the latest Electron versions.

@ofborg ofborg bot requested a review from yu-re-ka December 7, 2023 13:01
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 7, 2023
@dotlambda
Copy link
Member

Has anyone tried running Bitwarden under electron_26?

@dotlambda dotlambda mentioned this pull request Dec 10, 2023
13 tasks
@SuperSandro2000 SuperSandro2000 deleted the electron-25-source branch December 10, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants