Skip to content

Conversation

@Steinhagen
Copy link
Contributor

Update the package to the latest versions and remove the broken tag.

  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@Steinhagen Steinhagen requested review from dotlambda and pbsds April 22, 2025 21:50
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 22, 2025
@nix-owners nix-owners bot requested a review from SebTM April 22, 2025 21:58
@Steinhagen Steinhagen marked this pull request as draft April 22, 2025 22:01
@Steinhagen
Copy link
Contributor Author

.#python313Packages.proton-vpn-network-manager is still failing for the Python 3.13 version. It works as expected for the 3.12 version though. The update will be marked as draft until this issue is resolved.

@Steinhagen
Copy link
Contributor Author

The python mismatch is problematic, but doesn't block the final ProtonVPN build application. Since to my knowledge, there isn't any other application that depends on this library, I think we can remove the broken tag from it and track this issue separately in: #401088

@Steinhagen Steinhagen marked this pull request as ready for review April 23, 2025 10:04
'';

meta = {
broken = true; # ModuleNotFoundError: No module named 'proton.vpn.local_agent'
Copy link
Member

Choose a reason for hiding this comment

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

# https://github.com/NixOS/nixpkgs/issues/401088
# ModuleNotFoundError: No module named 'proton.vpn.local_agent'
broken = pythonAtLeast "3.13";

Copy link
Member

Choose a reason for hiding this comment

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

Please just fix proton-vpn-local-agent using

override { python3 = python; }

Copy link
Member

Choose a reason for hiding this comment

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

Actually, doesn't this PR already fix that issue?

Copy link
Member

@pbsds pbsds Apr 23, 2025

Choose a reason for hiding this comment

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

now it does yes, and that was the initial proposed fix in #400949 (comment)

@pbsds
Copy link
Member

pbsds commented Apr 23, 2025

A working fix is (proton-vpn-local-agent.override { python3 = python; }), but tbh proton-vpn-local-agent should be moved to pkgs/development/python-modules/

@pbsds
Copy link
Member

pbsds commented Apr 23, 2025

I pushed the fix, please test

@nix-owners nix-owners bot requested a review from natsukium April 23, 2025 16:02
Copy link
Contributor Author

@Steinhagen Steinhagen left a comment

Choose a reason for hiding this comment

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

Everything works as expected right now after the migration to of the local-agent package to python.

@dotlambda
Copy link
Member

Does proton-vpn-local-agent have an executable? If so please add mainProgram.

@Steinhagen
Copy link
Contributor Author

Does proton-vpn-local-agent have an executable? If so please add mainProgram.

To my knowledge, it doesn't. The main repository just compiles the library in rust and documents how to add it to your python site-packages.

@dotlambda
Copy link
Member

Does proton-vpn-local-agent have an executable? If so please add mainProgram.

To my knowledge, it doesn't. The main repository just compiles the library in rust and documents how to add it to your python site-packages.

Then why was it ever a top-level attribute?

@pbsds
Copy link
Member

pbsds commented Apr 23, 2025

Then why was it ever a top-level attribute?

I asked myself the very same question

@Steinhagen
Copy link
Contributor Author

Does proton-vpn-local-agent have an executable? If so please add mainProgram.

To my knowledge, it doesn't. The main repository just compiles the library in rust and documents how to add it to your python site-packages.

Then why was it ever a top-level attribute?

The fault for that is on me. I wasn't aware at the time of packaging that this will only be a python library. Sorry about this.

@pbsds
Copy link
Member

pbsds commented Apr 23, 2025

No worries 👍, we can't expect all contributors to be aware of all conventions! 😅
What surprises rather me is that this initially managed to slip through review

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.

Diff LGTM and it works on my machine.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 24, 2025
@Atemu Atemu merged commit b12f334 into NixOS:master Apr 24, 2025
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants