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

globalprotect-openconnect_2: init at 2.3.9 #350777

Closed
wants to merge 3 commits into from
Closed

Conversation

m1dugh
Copy link
Contributor

@m1dugh m1dugh commented Oct 23, 2024

The version 2 of globalprotect-openconnect

The package already exists in the registry, however, the version adds considerable breaking changes including
api keys for the frontend.

Things done

  • Built on platform(s)

    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:

    • x86_64-linux
    • aarch64-linux
  • 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.11 Release Notes (or backporting 23.11 and 24.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.


Add a 👍 reaction to [pull requests you find important].

@m1dugh m1dugh force-pushed the master branch 5 times, most recently from 01df7ea to a6d026b Compare October 25, 2024 10:10
@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 25, 2024

Result of nixpkgs-review pr 350777 run on x86_64-linux 1

@m1dugh m1dugh force-pushed the master branch 2 times, most recently from 10a1cd4 to f123c93 Compare October 25, 2024 10:52
@m1dugh m1dugh self-assigned this Oct 25, 2024
@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 25, 2024

Result of nixpkgs-review pr 350777 run on x86_64-linux 1

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 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: 1-10 10.rebuild-linux: 1 labels Oct 25, 2024
@m1dugh

This comment has been minimized.

@nixpkgs-merge-bot

This comment has been minimized.

@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 25, 2024

@tomodachi94 If ever you have time to check out this pr ❤️

@m1dugh m1dugh requested a review from tomodachi94 October 25, 2024 17:28
@tomodachi94
Copy link
Member

tomodachi94 commented Oct 25, 2024

I'll take a look in a little bit ❤️ thanks for the ping. Another good reviewer would be the maintainer of the regular (globalprotect-openconnect) package, if it exists.

@m1dugh m1dugh requested a review from jerith666 October 25, 2024 21:07
@jerith666
Copy link
Contributor

At first glance this looks like a partial duplicate of #316526 by @Binary-Eater, though that work didn't attempt to package the GUI.

Not sure whether it makes sense to try using this packaging of the GUI with the existing packaging of gpauth & gpclient, or if this packaging of everything is more self-consistent.

I haven't tried this locally yet but will try to find time for that soon.

@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 26, 2024

Turns out the gui cannot be compiled with rust as the sources are not available on the github.
Only the binary is available.

@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 26, 2024

I feel like, if the gui is used, it makes sense to have all the binaries in one package since gpclient relies on the rest of the binaries to work properly with the gui:

  • gpclient calls gpservice
  • which calls gpgui in turn
  • and so on

@m1dugh m1dugh requested a review from wegank October 26, 2024 13:31
@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 26, 2024

@wegank The whole package is released under GPL3.0 license I guess 🤔 ? Maybe we could merge your changes from #316526 into the same package, however, removing the original package is harsh for people using the gui without willing to pay for the license I feel .

@Binary-Eater
Copy link
Member

The license for the GUI component is not GPLv3 while the rest of the components are. That messiness is main reason why I avoided touching the GUI in packaging work for v2. Since v1 is unmaintained, I did not feel comfortable persisting it.

@Binary-Eater
Copy link
Member

Binary-Eater commented Oct 26, 2024

That said, I would be more keen on bringing back the v1 globalprotect-openconnect as a separate package than dealing with the proprietary licensed and paidware gui for v2.

@m1dugh
Copy link
Contributor Author

m1dugh commented Oct 26, 2024

Maybe we can change the license of the v2 full package to unfree, whilst keeping your packaged version of gpclient and gpauth as gpl3 packages, but I feel like the gui is a nice add to the packages and it requires gpservice which is unpackaged as of now.

Copy link
Member

@Binary-Eater Binary-Eater left a comment

Choose a reason for hiding this comment

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

The new changes do not work since the copied desktop entry will need to be patched using substituteInPlace.

❯ sudo -E gpclient connect --default-browser XXXX
Place your right index finger on the fingerprint reader
Failed to match fingerprint
Place your right index finger on the fingerprint reader
[2024-11-13T20:32:26Z INFO  gpclient::cli] gpclient started: 2.3.9 (2024-11-13)
[2024-11-13T20:32:26Z INFO  gpapi::portal::prelogin] Portal prelogin with user_agent: PAN GlobalProtect
[2024-11-13T20:32:26Z INFO  gpapi::portal::prelogin] Perform prelogin, user_agent: PAN GlobalProtect
[2024-11-13T20:32:28Z INFO  gpauth::cli] gpauth started: 2.3.9 (2024-11-13)
[2024-11-13T20:32:28Z INFO  gpapi::process::browser_authenticator] Launching the default browser...
[2024-11-13T20:32:28Z INFO  gpauth::cli] Please continue the authentication process in the default browser
[2024-11-13T20:32:28Z INFO  gpauth::cli] Listening authentication data on port 36861
[2024-11-13T20:32:28Z INFO  gpauth::cli] If it hangs, please check the logs at `/tmp/gpcallback.log` for more information
# Hangs here

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 13, 2024

Turns out there is a bug running the gui with webkitgtk version 2.46.3+abi=4.0 which causes the application to crash. This is caused by webkitgtk lib only. To fix it, one needs to override the webkitgtk version to 2.44.3+abi=4.0 which is the current version in nixos-24.05 😞 .

@tomodachi94
Copy link
Member

My suggestion would be to file an issue upstream, and go from there.

@Binary-Eater
Copy link
Member

Yeah, I never felt comfortable with chucking the GUI into nixpkgs given it's proprietary, hard to patch, and dependent on webkitgtk (so it's built against a fixed version of it most likely and shipped). It seems like a huge maintenance cost. Even packages like protonmail-bridge strip their GUI components in nixpkgs.

@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 13, 2024

The GUI still looks like a nice feature to have, especially since we are replacing the old version of the package which had the GUI I think 🤔 .

@Binary-Eater
Copy link
Member

I would be ok with bringing back v1 which was fully open (so patching is an option) rather than trying to replace it with v2's GUI (where a paid license is required to use it anyways).

@Binary-Eater
Copy link
Member

Just opened #355758

@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 13, 2024

And some people might be willing to pay to use the GUI I guess, maybe, the right way is to bring back the v1 to life, i don't know.

@Binary-Eater
Copy link
Member

Yeah, my issue is having a bunch of broken packages in nixpkgs. I think keeping v1's GUI workable is viable, but I do not feel comfortable about v2. The nice thing about nixos is it is trivial to include external packaging. I think nixpkgs at the very least should contain working packages that are not trivially susceptible to breaking on upgrades and do not need pinned dependencies.

@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 13, 2024

That's a fair point

@Binary-Eater
Copy link
Member

I can add you as a maintainer on the existing in-tree packages for v2 as well (I would actually really appreciate that if you do not mind).

@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 13, 2024

That would be very nice.

Binary-Eater added a commit to Binary-Eater/nixpkgs that referenced this pull request Nov 13, 2024
@m1dugh has contributed a lot in NixOS#350777. The upstream state of the GUI
component combined with the paid licensing model has made it daunting to
consider integrating into nixpkgs. Instead, add @m1dugh as a co-maintainer
for the existing v2 packages.

Signed-off-by: Rahul Rameshbabu <[email protected]>
Binary-Eater added a commit to Binary-Eater/nixpkgs that referenced this pull request Nov 13, 2024
@m1dugh has contributed a lot in NixOS#350777. The upstream state of the GUI
component combined with the paid licensing model has made it daunting to
consider integrating into nixpkgs. Instead, add @m1dugh as a co-maintainer
for the existing v2 packages.

Signed-off-by: Rahul Rameshbabu <[email protected]>
@Binary-Eater
Copy link
Member

Just opened #355768.

@jerith666
Copy link
Contributor

This will conflict with both #355758 and #355768 when they merge, I believe. So I think it makes sense to let those merge first and then deal with the conflicts here, esp. since it sounds like there are still unresolved issues with webkitgtk here anyway, correct?

Binary-Eater added a commit to Binary-Eater/nixpkgs that referenced this pull request Nov 14, 2024
@m1dugh has contributed a lot in NixOS#350777. The upstream state of the GUI
component combined with the paid licensing model has made it daunting to
consider integrating into nixpkgs. Instead, add @m1dugh as a co-maintainer
for the existing v2 packages.

Signed-off-by: Rahul Rameshbabu <[email protected]>
@m1dugh
Copy link
Contributor Author

m1dugh commented Nov 14, 2024

I guess we can just close this one now regarding @Binary-Eater comments on package being broken for gui.

@Binary-Eater
Copy link
Member

We can in parallel explore reaching out to the upstream maintainer and revive this PR if we see enough interest. Does that seem reasonable as an action plan?

@jerith666
Copy link
Contributor

That works for me.

@Binary-Eater
Copy link
Member

Going to close this PR now that the other two have merged. We can revisit this later on if need be/desired.

hatch01 pushed a commit to hatch01/nixpkgs that referenced this pull request Dec 7, 2024
@m1dugh has contributed a lot in NixOS#350777. The upstream state of the GUI
component combined with the paid licensing model has made it daunting to
consider integrating into nixpkgs. Instead, add @m1dugh as a co-maintainer
for the existing v2 packages.

Signed-off-by: Rahul Rameshbabu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 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.

6 participants