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

firefoxpwa: init at 2.11.1 #263404

Merged
merged 1 commit into from
Mar 25, 2024
Merged

firefoxpwa: init at 2.11.1 #263404

merged 1 commit into from
Mar 25, 2024

Conversation

camillemndn
Copy link
Contributor

@camillemndn camillemndn commented Oct 25, 2023

Description of changes

Fixes #215905

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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2859

@pasqui23
Copy link
Contributor

It should be noted that despite the various firefox.enableXXX options being deprecated we still would need such an option for firefoxpwa as it needs to be both in the plugins directory and in PATH

@camillemndn
Copy link
Contributor Author

It should be noted that despite the various firefox.enableXXX options being deprecated we still would need such an option for firefoxpwa as it needs to be both in the plugins directory and in PATH

Agreed, I think the way to go is:
nativeMessagingHosts.packages = [ pkgs.firefoxpwa ];
and
environment.systemPackages = [ pkgs.firefoxpwa ];

For Home Manager I am unsure the Firefox module was changed to match Nixpkgs'.

@camillemndn

This comment was marked as resolved.

@pasqui23
Copy link
Contributor

pasqui23 commented Dec 5, 2023 via email

@tukanoidd
Copy link

tukanoidd commented Dec 29, 2023

Hey, didn't realize this was here so I'm gonna just copy-paste what I wrote in here

https://github.com/Rutherther/nur-pkgs/tree/master/pkgs/firefoxpwa found it packaged by someone in NUR, but outdated (2.7.3, current 2.9.1), would it be possible to get it working based on this? I managed to get it installed and was able to get through connector recognition phase and even runtime installation, but wasn't able to actually run a PWA that I successfully installed according to the extension, just wasn't starting up (microsoft teams in my case), but I'm not sure if it's because it's outdated or because something else there is broken

Here's minimal working config (using HM, but should work without, I just modified my own config a bit, stripping out unneeded stuff):

home.packages = with pkgs; [
	nur.repos.rutherther.firefoxpwa
];
programs.firefox = {
	enable = true;
    package = pkgs.firefox.override {
      	nativeMessagingHosts = with pkgs; [
        	nur.repos.rutherther.firefoxpwa-unwrapped
		];
	};
};

I really like firefox (or floorp, override also works there btw, started using it recently, cuz after using Vivaldi, can't live without sidebar, but I really prefer firefox with it's performance and degree of additional customizability I can get with extensions) and the only thing that keeps me from using it 100% of the time is the lack of PWA support, which this extension could remedy.

EDIT: PWA launches only if run through CLI, but not browser UI, although I'm getting this:

[GFX1-]: glxtest: libpci missing
[GFX1-]: vaapitest: ERROR
[GFX1-]: vaapitest: VA-API test failed: libva-drm.so.2 is missing.

But I'm assuming it's not as big of a deal since it still worked?

@camillemndn

This comment was marked as resolved.

@tukanoidd
Copy link

Hi, did you actually try this PR? It has been perfectly functional as is for months now on my machines. I would really like to see it merged.

My bad, I saw that this issue has been here a while, so I (stupidly) assumed that there were still issues with packaging (judging by the issue upstream as well). If it works correctly then my question isn't valid anymore :)

@camillemndn camillemndn changed the title firefoxpwa: init at 2.8.0 firefoxpwa: init at 2.9.1 Jan 3, 2024
@camillemndn camillemndn changed the title firefoxpwa: init at 2.9.1 firefoxpwa: init at 2.10.0 Jan 22, 2024
@camillemndn
Copy link
Contributor Author

camillemndn commented Feb 4, 2024

@filips123 Hi, I will wrap the firefoxpwa-connector as well. Maybe there is a way to use directly the wrapped version of Firefox as runtime?

In NixOS, the updates are done by opening PRs in this repository. There are bots (@r-ryantm) to automate this, and script like nix-update.

Exactly, one only need to add these two options to be ready to go, that's what's great about NixOS!

I am working to incorporate your new feature firefoxpwa runtime patch. Thank you!

@filips123
Copy link

In NixOS, the updates are done by opening PRs in this repository.

For PWAsForFirefox, the extension and the native versions should be mostly in sync, because otherwise, there might be incompatibilities between them. So, if possible, I would like all updates to this package would also get backported to the stable channels when they are released on the unstable. The backporting criteria say that updates for "services that would fail without up-to-date client software" can be backported, so I think this is also applicable to PWAsForFirefox.

If this is not possible, then users should be instructed to use this package from the unstable branch, so it's always up-to-date.


Also, should mainProgram in meta be set to firefoxpwa?

@camillemndn camillemndn force-pushed the firefoxpwa branch 2 times, most recently from 31d2a22 to bc58eb4 Compare February 6, 2024 16:42
@ofborg ofborg bot requested a review from pasqui23 February 6, 2024 17:05
@filips123
Copy link

Is there any way to run firefoxpwa runtime patch automatically when the firefox package is updated? Because I think that now, when firefox is updated, the PWAsForFirefox runtime stored at $out/share/firefoxpwa/runtime will remain the old one, unless the user manually runs firefoxpwa runtime patch each time after firefox is updated.

@camillemndn
Copy link
Contributor Author

Ok, now firefoxpwa uses the upstream version of Firefox (it patches it at build time), thanks to the new immutable-runtime flag and the command firefoxpwa runtime patch which is used at build time.

@filips123
Copy link

Yeah, but if Firefox itself is updated after firefoxpwa has already been built and installed, won't the PWAsForFirefox runtime still remain on the old Firefox version?

@LennyPenny
Copy link

LennyPenny commented Feb 6, 2024

On nixos your entire system is always built from the same nixpkgs source at a certain commit (usually refrenced by their release tags like nixos-23.11 or nixpkgs-unstable), hence the packages will always be in sync.

There are ways you can use different packages from different nixpkgs versions, but resolving issues arising from that is up to the user. (i.e. using firefox from nixpkgs-unstable and firefoxpwa from nixos-23.11 could obviously lead to problems)

@camillemndn
Copy link
Contributor Author

camillemndn commented Feb 6, 2024

If you want to automatically install the addon:

programs.firefox.policies = {
  ExtensionSettings."firefoxpwa@filips.si" = {
    installation_mode = "force_installed";
    install_url = "https://addons.mozilla.org/firefox/downloads/latest/pwas-for-firefox";
  };
};

(Though this solution has some drawbacks, it seems to want to install the addon inside each web app!)

@filips123
Copy link

On nixos your entire system is always built from the same nixpkgs source at a certain commit (usually refrenced by their release tags like nixos-23.11 or nixpkgs-unstable), hence the packages will always be in sync.

Then, it seems fine to me. In the future, I will probably change a bit how immutable-runtime, which might require some small changes here, but for now it's fine.

using firefox from nixpkgs-unstable and firefoxpwa from nixos-23.11 could obviously lead to problems

What if firefoxpwa is from unstable and firefox is from stable? And if both are from unstable?

Though this solution has some drawbacks, it seems to want to install the addon inside each web app!

It's probably better then to let users manually install the addon.

FFPWA_SYSDATA = "${placeholder "out"}/share/firefoxpwa";
completions = "target/${stdenv.targetPlatform.config}/release/completions";

gtk_modules = map (x: x + x.gtkModule) [ libcanberra-gtk3 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gtk_modules = map (x: x + x.gtkModule) [ libcanberra-gtk3 ];
gtk_modules = [ libcanberra-gtk3.gtkModule ];

, cups
, pciutils
, libcanberra-gtk3
, extraLibs ? [ ]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this parameter for?

completions = "target/${stdenv.targetPlatform.config}/release/completions";

gtk_modules = map (x: x + x.gtkModule) [ libcanberra-gtk3 ];
libs = let libs = lib.optionals stdenv.isLinux [ udev libva mesa libnotify xorg.libXScrnSaver cups pciutils ] ++ gtk_modules ++ extraLibs; in lib.makeLibraryPath libs + ":" + lib.makeSearchPathOutput "lib" "lib64" libs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
libs = let libs = lib.optionals stdenv.isLinux [ udev libva mesa libnotify xorg.libXScrnSaver cups pciutils ] ++ gtk_modules ++ extraLibs; in lib.makeLibraryPath libs + ":" + lib.makeSearchPathOutput "lib" "lib64" libs;
libs = let libs = lib.optionals stdenv.isLinux [ udev libva mesa libnotify xorg.libXScrnSaver cups pciutils ] ++ gtk_modules ++ extraLibs ++ lib.makeSearchPathOutput "lib" "lib64" libs; in lib.makeLibraryPath libs;

firefoxpwa: move to pkgs/by-name
@adamcstephens adamcstephens changed the title firefoxpwa: init at 2.10.1 firefoxpwa: init at 2.11.1 Mar 25, 2024
@ofborg ofborg bot requested a review from pasqui23 March 25, 2024 13:43
@adamcstephens adamcstephens merged commit 8893c58 into NixOS:master Mar 25, 2024
25 of 26 checks passed
@KiaraGrouwstra
Copy link
Contributor

Agreed, I think the way to go is:

nativeMessagingHosts.packages = [ pkgs.firefoxpwa ];
environment.systemPackages = [ pkgs.firefoxpwa ];

For Home Manager I am unsure the Firefox module was changed to match Nixpkgs'.

it works similarly there with this PR:

home.packages = [ pkgs.firefoxpwa ];
programs.firefox.nativeMessagingHosts = [ pkgs.firefoxpwa ];

@adamcstephens
Copy link
Contributor

You can also just override the firefox wrapper to add the extension host. I used this with HM since I don't use the firefox option, but would work equally well with environment.systemPackages

home.packages = [
  pkgs.firefoxpwa
  (pkgs.firefox-wayland.override { nativeMessagingHosts = [ pkgs.firefoxpwa ]; })
];

@Thatoo
Copy link

Thatoo commented Apr 4, 2024

@adamcstephens could it be backported to 23.11?

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

10 participants