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

[20.09] firefox: enable pipewire+webrtc for wayland users #99692

Merged
merged 2 commits into from Oct 7, 2020

Conversation

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Oct 6, 2020

(cherry picked from commit 31e54cd)

Motivation for this change

Backports #84233

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
  • 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 nixpkgs-review --run "nixpkgs-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.
(cherry picked from commit 31e54cd)
@worldofpeace worldofpeace changed the title firefox: enable pipewire+webrtc for wayland users [20.09] firefox: enable pipewire+webrtc for wayland users Oct 6, 2020
@worldofpeace worldofpeace mentioned this pull request Oct 6, 2020
0 of 10 tasks complete
Copy link
Contributor

@endgame endgame left a comment

I don't know the firefox build system so some of these comments might be off-base.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Oct 6, 2020

@endgame This is a backport. That means you really can't request changes to already committed code without encouraging diversion which has to be forward ported.

@endgame
Copy link
Contributor

@endgame endgame commented Oct 6, 2020

@worldofpeace Fair enough then.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Oct 6, 2020

@endgame Not sure if you could submit a review the original merged PR, though. Let's see what @andir thinks

@endgame
Copy link
Contributor

@endgame endgame commented Oct 6, 2020

I tried to build firefox from nixpkgs master without wayland and it broke because it couldn't find -lpipewire so something needs to be reported: either on the original PR or as a new issue. No point pretending that wayland is an option if it breaks when turned off.

@andir
Copy link
Member

@andir andir commented Oct 6, 2020

Thank you for testing and reporting @endgame.

@worldofpeace due to the quality of the patch I didn't want it in the first place but was probably too inactive to make my opinion voiced enough. At this stage I'd at least refrain from adding it to the stable release. It is a nice feature and a pity but I do not see us having the manpower to do fix and support this patch.

I'll spend a bit of time trying to clean the patch up to fix the situation in both master and this backport. If that fails I'm still in favor of skipping it or making the entire code optional (which is a mess I really dislike).

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Oct 6, 2020

This patch is actually being used in more distros than I thought, see https://github.com/emersion/xdg-desktop-portal-wlr/wiki/Screencast-Compatibility#webrtc-aka-firefoxchromium

This ensures that we aren't applying any of the experiemental pipewire
patches when the dependencies aren't enabled. As of now pipewire only
works with wayland and webrtc. If either of them are not activated we
can't build with pipewireSupport and we should not.
@andir
Copy link
Member

@andir andir commented Oct 6, 2020

This patch is actually being used in more distros than I thought, see https://github.com/emersion/xdg-desktop-portal-wlr/wiki/Screencast-Compatibility#webrtc-aka-firefoxchromium

Yeah but most of them hide it behind a feature flag and/or only provide a single build for their entire distribution. Here we are trying to allow users to customize the package but didn't bake that into the expression when this patch was added.

I just pushed c9c0b90 to this branch which does the required housekeeping for the added patch. I'll also add this to unstable once we agree on it.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Oct 6, 2020

This patch is actually being used in more distros than I thought, see https://github.com/emersion/xdg-desktop-portal-wlr/wiki/Screencast-Compatibility#webrtc-aka-firefoxchromium

Yeah but most of them hide it behind a feature flag and/or only provide a single build for their entire distribution. Here we are trying to allow users to customize the package but didn't bake that into the expression when this patch was added.

I just pushed c9c0b90 to this branch which does the required housekeeping for the added patch. I'll also add this to unstable once we agree on it.

Thanks, it looks good.

@worldofpeace
Copy link
Member Author

@worldofpeace worldofpeace commented Oct 6, 2020

@andir Could you also summarize your current stance on this? It seems you've taken a few and it's not clearly linear because some of it is in the review comments.

@andir
Copy link
Member

@andir andir commented Oct 6, 2020

This PR as is is probably fine. If you want to carry that into a release that sounds fine. I still worry about keeping the feature stable as we effectively now depend on Mozilla and a single fedora maintainer. Given the history with pipewire fedora will probably continue to provide patches. I just hope they'll do it on time as very often they were slower than us in updating the browser when security issues appeared.

@andir andir merged commit 8c2b1d4 into NixOS:release-20.09 Oct 7, 2020
16 checks passed
16 checks passed
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
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="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./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="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="c9c0b90"; rev="c9c0b90b62bb863b56a7bdc356d38f7b143f2fa5"; } ./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
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

4 participants
You can’t perform that action at this time.