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

protonvpn-gui: 4.4.4 -> 4.6.0 #343204

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Conversation

Steinhagen
Copy link
Contributor

Description of changes

Update the package to the latest version.

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.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.

@SebTM
Copy link
Contributor

SebTM commented Sep 20, 2024

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

26 packages built:
  • protonvpn-gui
  • protonvpn-gui.dist
  • python311Packages.proton-vpn-api-core
  • python311Packages.proton-vpn-api-core.dist
  • python311Packages.proton-vpn-killswitch-network-manager
  • python311Packages.proton-vpn-killswitch-network-manager-wireguard
  • python311Packages.proton-vpn-killswitch-network-manager-wireguard.dist
  • python311Packages.proton-vpn-killswitch-network-manager.dist
  • python311Packages.proton-vpn-network-manager
  • python311Packages.proton-vpn-network-manager-openvpn
  • python311Packages.proton-vpn-network-manager-openvpn.dist
  • python311Packages.proton-vpn-network-manager-wireguard
  • python311Packages.proton-vpn-network-manager-wireguard.dist
  • python311Packages.proton-vpn-network-manager.dist
  • python312Packages.proton-vpn-api-core
  • python312Packages.proton-vpn-api-core.dist
  • python312Packages.proton-vpn-killswitch-network-manager
  • python312Packages.proton-vpn-killswitch-network-manager-wireguard
  • python312Packages.proton-vpn-killswitch-network-manager-wireguard.dist
  • python312Packages.proton-vpn-killswitch-network-manager.dist
  • python312Packages.proton-vpn-network-manager
  • python312Packages.proton-vpn-network-manager-openvpn
  • python312Packages.proton-vpn-network-manager-openvpn.dist
  • python312Packages.proton-vpn-network-manager-wireguard
  • python312Packages.proton-vpn-network-manager-wireguard.dist
  • python312Packages.proton-vpn-network-manager.dist

@SebTM
Copy link
Contributor

SebTM commented Sep 20, 2024

Haven't tested it yet, might depend/merged together with #343131, #343208

@Steinhagen
Copy link
Contributor Author

Gave it a few tries and from what I've observed the only dependency is for proton-vpn-api-core to be updated at the same time with protonvpn-app to 4.4.5.

This is why this PR contains 2 commits.

That being said, I haven't tested all possibilities, so indeed, might be a good idea to update everything at once.

@SebTM
Copy link
Contributor

SebTM commented Sep 20, 2024

There is some issue when running all updated packages together and only using wireguard:

2024-09-20T12:22:23.944299 | asyncio:1820 | ERROR | Exception in callback AgentListener._on_background_task_stopped(<Task finishe...net failed.')>)
handle: <Handle AgentListener._on_background_task_stopped(<Task finishe...net failed.')>)>
Traceback (most recent call last):
  File "/nix/store/wlnlykpg9pwb0zxiv2m12slm65p21sra-python3.12-proton-vpn-network-manager-0.6.3/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/protocol/wireguard/local_agent/fallback_local_agent.py", line 129, in connect
    return await loop.run_in_executor(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
TimeoutError: timed out

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/nix/store/h3i0acpmr8mrjx07519xxmidv8mpax4y-python3-3.12.5/lib/python3.12/asyncio/events.py", line 88, in _run
    self._context.run(self._callback, *self._args)
  File "/nix/store/wlnlykpg9pwb0zxiv2m12slm65p21sra-python3.12-proton-vpn-network-manager-0.6.3/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/protocol/wireguard/local_agent/listener.py", line 130, in _on_background_task_stopped
    background_task.result()
  File "/nix/store/wlnlykpg9pwb0zxiv2m12slm65p21sra-python3.12-proton-vpn-network-manager-0.6.3/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/protocol/wireguard/local_agent/listener.py", line 72, in _run_in_background
    self._connection = await self._connector.connect(domain, credentials)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wlnlykpg9pwb0zxiv2m12slm65p21sra-python3.12-proton-vpn-network-manager-0.6.3/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/protocol/wireguard/local_agent/fallback_local_agent.py", line 133, in connect
    raise LocalAgentError(
proton.vpn.backend.linux.networkmanager.protocol.wireguard.local_agent.fallback_local_agent.LocalAgentError: Local agent connection to node-de-19.protonvpn.net failed.

A new (partial) rust-module with python-bindings got released here - so I guess we need this one for the update as well: https://github.com/ProtonVPN/python-proton-vpn-local-agent

@Steinhagen
Copy link
Contributor Author

Steinhagen commented Sep 20, 2024

There isn't a tag/release for that yet. Should we pack that using the latest current commit?
Another good question would be how can that be packaged correctly in Nix?

@SebTM
Copy link
Contributor

SebTM commented Sep 20, 2024

I've checked that with the team, we can use the latest commit - for the python-bindings cargo build --release should be enough, for the rust library the build process should be sth. like this:

cd python-proton-vpn-local-agent 
cargo build --release 
mv target/release/libpython_proton_vpn_local_agent.so local_agent.so

@Steinhagen
Copy link
Contributor Author

I've checked that with the team, we can use the latest commit - for the python-bindings cargo build --release should be enough, for the rust library the build process should be sth. like this:

cd python-proton-vpn-local-agent 
cargo build --release 
mv target/release/libpython_proton_vpn_local_agent.so local_agent.so

Can you push a commit for this on the PR branch?

@SebTM
Copy link
Contributor

SebTM commented Sep 22, 2024

Hey, I gave it a try but could use some help here - I can see when building local_agent_rs with a nix-shell with cargo that the build-output is in target/release/ and:

.rw-r--r--   959 sebtm 22 Sep 10:38   -I  liblocal_agent_rs.d
.rw-r--r--  731k sebtm 22 Sep 10:38   -I  liblocal_agent_rs.rlib

but when building it with nix the folder contains only the other also present data not the build artifact - maybe I'm doing something wrong here not sure:

dr-xr-xr-x     - root   1 Jan  1970   .fingerprint
dr-xr-xr-x     - root   1 Jan  1970   build
dr-xr-xr-x     - root   1 Jan  1970   deps
dr-xr-xr-x     - root   1 Jan  1970   examples
dr-xr-xr-x     - root   1 Jan  1970   incremental
.r--r--r--     0 root   1 Jan  1970   .cargo-lock

(These are also present when building with nix-shell)

Building the ".so" file for the python-bindings worked easily - prepared the code to merge them into one package I would expect that makes sense as they are in the same repo?

You can reproduce my nixpkgs-build state with tmp_protonvpn-lad or see the relevant changes I tried here: 189430f

@Steinhagen
Copy link
Contributor Author

Building the ".so" file for the python-bindings worked easily - prepared the code to merge them into one package I would expect that makes sense as they are in the same repo?

I also think that they should be in a single package since that is also the way they were packaged upstream.

Regarding the build artifacts, I don't know the reason why that is happening or have an idea. Sorry about this.

P.S.: I know that is work in progress, but on the changes branch, the meta was not updated for the packages.

@Steinhagen
Copy link
Contributor Author

Steinhagen commented Sep 23, 2024

I was wrong. With the merge of #343208, it seems that protonvpn is no longer working on master without updating protonvpn-gui as well.

@SebTM
Copy link
Contributor

SebTM commented Sep 25, 2024

I was wrong. With the merge of #343208, it seems that protonvpn is no longer working on master without updating protonvpn-gui as well.

I would advise in such situations to create a revert PR and ping in the original ones. Another one seems to got merged as well today. I'm currently not able to respond in short time.


I've got the info we only need to build python-proton-vpn-local-agent because if you look in their Cargo.toml file you see:

[dependencies]
local-agent-rs = { path = "../local_agent_rs" }

the local_agent_rs is included in the built-result. This said I updated my branch with this changes and added the new package to protonvpn-gui and proton-vpn-network-manager but it appears to me no difference without the package so there might be an issues with wrapping or something else that it's not finding/picking up the ".so"-file?

There seems to be no difference in putting the build-result in $out or $out/lib.

I've also asked here in matrix for help on this issue: https://matrix.to/#/!FBuJyWXTGcGtHTPphC:nixos.org/$Fl_LLhWapJS740g-yQKYZDOpzfIz2ah8XBazSylfqOA?via=nixos.org&via=matrix.org&via=envs.net

@Steinhagen
Copy link
Contributor Author

Steinhagen commented Sep 25, 2024

Based on the install scripts located in the python-proton-vpn-local-agent git repository, I've installed the library in the same location where the scripts would have.

Unfortunately, I can't replicate the issue, but can you give it a try and see if it's still reproducing? I've added the library commit on top of this PR.

As a related note, there were some new package updates that target Wireguard, so I've updated everything again in this PR as well. Please, let me know if this works for you.

@Steinhagen Steinhagen changed the title protonvpn-gui: 4.4.4 -> 4.4.5 protonvpn-gui: 4.4.4 -> 4.5.0 Sep 25, 2024
@Steinhagen Steinhagen force-pushed the protonvpn-gui-4-4-5 branch 2 times, most recently from b630c94 to 9a6800e Compare September 25, 2024 20:16
@SebTM
Copy link
Contributor

SebTM commented Sep 25, 2024

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

19 packages built:
  • proton-vpn-local-agent
  • protonvpn-gui
  • protonvpn-gui.dist
  • python311Packages.proton-vpn-api-core
  • python311Packages.proton-vpn-api-core.dist
  • python311Packages.proton-vpn-killswitch-network-manager
  • python311Packages.proton-vpn-killswitch-network-manager-wireguard
  • python311Packages.proton-vpn-killswitch-network-manager-wireguard.dist
  • python311Packages.proton-vpn-killswitch-network-manager.dist
  • python311Packages.proton-vpn-network-manager
  • python311Packages.proton-vpn-network-manager.dist
  • python312Packages.proton-vpn-api-core
  • python312Packages.proton-vpn-api-core.dist
  • python312Packages.proton-vpn-killswitch-network-manager
  • python312Packages.proton-vpn-killswitch-network-manager-wireguard
  • python312Packages.proton-vpn-killswitch-network-manager-wireguard.dist
  • python312Packages.proton-vpn-killswitch-network-manager.dist
  • python312Packages.proton-vpn-network-manager
  • python312Packages.proton-vpn-network-manager.dist

@SebTM
Copy link
Contributor

SebTM commented Sep 25, 2024

Unfortunately, I can't replicate the issue, but can you give it a try and see if it's still reproducing? I've added the library commit on top of this PR.

You are right, I could reproduce it working with the reproduced directory structure - thought that's somehow not needed/handled by nix as it prefers (without renaming) to put it into "lib/name.so".

Again something learned.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

A few nits about the diffs, otherwise LGTM and this works for me in WireGuard mode.

@Steinhagen Steinhagen force-pushed the protonvpn-gui-4-4-5 branch 2 times, most recently from c13d610 to d3e30e5 Compare October 16, 2024 12:49
@Steinhagen Steinhagen requested a review from Atemu October 16, 2024 12:51
@Steinhagen
Copy link
Contributor Author

Simplified the commit tree based on your suggestions. Please give it a try to see if everything is still working as expected.

@Steinhagen
Copy link
Contributor Author

This should be merged only after #349048 gets merged.

@Steinhagen Steinhagen marked this pull request as draft October 16, 2024 14:27
@Steinhagen Steinhagen marked this pull request as ready for review October 16, 2024 20:45
@Steinhagen Steinhagen removed 8.has: package (new) This PR adds a new package 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 17, 2024
@Steinhagen
Copy link
Contributor Author

#349048 was merged. I rebased this PR and verified that everything still works as expected.

Atemu
Atemu previously requested changes Oct 17, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Hm, I'm getting this error now when I try to connect via WG:

2024-10-17T09:20:40.882006 | proton.vpn.backend.linux.networkmanager.core.networkmanager:130 | ERROR | Error starting NetworkManager connection.
Traceback (most recent call last):
  File "/nix/store/zj9zckgxzbm6dq6psi0s91ia1s9j0lwy-python3.12-proton-vpn-network-manager-0.9.1/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/core/networkmanager.py", line 121, in start
    vpn_connection = await loop.run_in_executor(
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/_base.py", line 456, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/nix/store/zj9zckgxzbm6dq6psi0s91ia1s9j0lwy-python3.12-proton-vpn-network-manager-0.9.1/lib/python3.12/site-packages/proton/vpn/backend/linux/networkmanager/core/nmclient.py", line 131, in callback
    result = getattr(source_object, finish_method_name)(res)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
gi.repository.GLib.GError: nm-manager-error-quark: Authorization request cancelled (1)
2024-10-17T09:20:41.021725 | asyncio:1820 | ERROR | Exception in callback Publisher._on_notification_task_done(<Task finishe...seException')>)
handle: <Handle Publisher._on_notification_task_done(<Task finishe...seException')>)>
Traceback (most recent call last):
  File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/asyncio/events.py", line 88, in _run
    self._context.run(self._callback, *self._args)
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/connection/publisher.py", line 92, in _on_notification_task_done
    task.result()
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/core/connection.py", line 439, in _on_connection_event
    event = await self._handle_on_event(event)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/core/connection.py", line 422, in _handle_on_event
    raise excp
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/core/connection.py", line 414, in _handle_on_event
    new_state = self.current_state.on_event(event)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/connection/states.py", line 120, in on_event
    event.check_for_errors()
  File "/nix/store/npmq55wb7dvf5c3j8qy3xx1ahhgyw2d8-python3.12-proton-vpn-api-core-0.35.5/lib/python3.12/site-packages/proton/vpn/connection/events.py", line 62, in check_for_errors
    raise self.context.error
TypeError: exceptions must derive from BaseException

@Steinhagen
Copy link
Contributor Author

Steinhagen commented Oct 17, 2024

Weird. It worked on my side without any issues. From what I've seen there are some issues where the application is not starting as expected (I expect some race condition in the app itself), but other than that, I haven't seen anything.

I also did a diff between the previous working version before the commit squash and this one and they are identical.

image

I know it's annoying, but can you try to log in again in the app?

@Steinhagen Steinhagen requested a review from Atemu October 17, 2024 18:36
@Atemu
Copy link
Member

Atemu commented Oct 18, 2024

Nope, same issue.

As it turns out, this is actually caused by the rebase onto a newer nixpkgs. When I backport the working PR's commits (eccba9f) onto unstable, it also doesn't work anymore.

@Atemu
Copy link
Member

Atemu commented Oct 18, 2024

nm.log

Update to the latest version.
Update the package to the latest version.
Update to the latest versions.

The proton-vpn-network-manager-wireguard and proton-vpn-network-manager-openvpn
modules are now legacy: The same functionality is now in proton-vpn-network-manager module,
version 0.6.3 and upwards.
Update the package to the latest version.
Update the protonvpn-gui package to the latest version.

The proton-vpn-network-manager-wireguard and proton-vpn-network-manager-openvpn
modules are now legacy: The same functionality is now in proton-vpn-network-manager
module, version 0.6.3 and upwards.

proton-vpn-connection functionality was integrated in the proton-vpn-api-core module.
proton-vpn-session functionality was integrated in the proton-vpn-api-core module.
proton-vpn-killswitch functionality was integrated in the proton-vpn-api-core module.
proton-keyring-linux-secretservice functionality was integrated in the proton-keyring-linux module
proton-vpn-killswitch-network-manager-wireguard functionality was integrated in the proton-vpn-network-manager module
proton-vpn-killswitch-network-manager functionality was integrated in the proton-vpn-network-manager module
proton-vpn-logger functionality was integrated in the proton-vpn-api-core module
@Steinhagen
Copy link
Contributor Author

Steinhagen commented Oct 18, 2024

I rebased it on the latest master branch and it still works on my machine. I don't know exactly why this is happening on your machine: platform-linux: wireguard: set-device, message #0 was rejected: Address already in use
I don't know exactly what to do at the present time regarding this PR. We can also wait until someone else who has ProtonVPN tests it and see if it works as expected, or wait until a new version and see if this is fixed by something in upstream.

That being said, I don't feel comfortable pushing this into master until we have a clear confirmation that the package works as expected for everybody (and hopefully root-cause the previously mentioned issue).

@SebTM
Copy link
Contributor

SebTM commented Oct 21, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 343204


x86_64-linux

✅ 18 packages built:
  • protonvpn-gui
  • protonvpn-gui.dist
  • python311Packages.proton-core
  • python311Packages.proton-core.dist
  • python311Packages.proton-keyring-linux
  • python311Packages.proton-keyring-linux.dist
  • python311Packages.proton-vpn-api-core
  • python311Packages.proton-vpn-api-core.dist
  • python311Packages.proton-vpn-network-manager
  • python311Packages.proton-vpn-network-manager.dist
  • python312Packages.proton-core
  • python312Packages.proton-core.dist
  • python312Packages.proton-keyring-linux
  • python312Packages.proton-keyring-linux.dist
  • python312Packages.proton-vpn-api-core
  • python312Packages.proton-vpn-api-core.dist
  • python312Packages.proton-vpn-network-manager
  • python312Packages.proton-vpn-network-manager.dist

Copy link
Contributor

@SebTM SebTM left a comment

Choose a reason for hiding this comment

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

LGTM, tested OpenVPN (TCP/UDP) and wireguard against unstable:

 - system: `"x86_64-linux"`
 - host os: `Linux 6.11.4, NixOS, 24.11 (Vicuna), 24.11.20241021.ce1b562`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Lix, like Nix) 2.91.0
System type: x86_64-linux
Additional system types: aarch64-linux, i686-linux
Features: gc, signed-caches
System configuration file: /etc/nix/nix.conf
User configuration files: /home/sebtm/.config/nix/nix.conf:/etc/xdg/nix/nix.conf:/home/sebtm/.nix-profile/etc/xdg/nix/nix.conf:/nix/profile/etc/xdg/nix/nix.conf:/home/sebtm/.local/state/nix/profile/etc/xdg/nix/nix.conf:/etc/profiles/per-user/sebtm/etc/xdg/nix/nix.conf:/nix/var/nix/profiles/default/etc/xdg/nix/nix.conf:/run/current-system/sw/etc/xdg/nix/nix.conf:/nix/store/rjvx7znypvvircyfz99pa00a3a596661-gnome-settings-daemon-46.0/etc/xdg/nix/nix.conf
Store directory: /nix/store
State directory: /nix/var/nix
Data directory: /nix/store/g839w7gjl0v4vc1gc9gd80ns57hb2kgm-lix-2.91.0/share`
 - nixpkgs: `/nix/store/g9n71srms32l7p3s21vbk9az587fjf2l-source`

worked for me without issues.


You can try to delete the cache if you switch between versions (especially multiple releases): ~/.cache/Proton/VPN/

If there is a persistent issue ~/.cache/Proton/VPN/logs/vpn-app.log and journalctl -u NetworkManager / nmcli c monitor can be helpful.

@Atemu
Copy link
Member

Atemu commented Oct 21, 2024

The trouble is that it worked on this same versoin but with an older state of the rest of nixpkgs which is very weird.

If it still works for you, this is probably a me issue though.

@Atemu Atemu dismissed their stale review October 21, 2024 16:01

Works for others

@Atemu Atemu merged commit 26619c1 into NixOS:master Oct 21, 2024
28 checks passed
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.

5 participants