cc openconnect maintainers @pradeepchhetri @tricktron

Thanks for the thorough review, @SuperSandro2000. I did some fetching and rebasing so I could be clear on which of your suggestions were semantic versus only formatting. I've included the semantic suggestions and the two fixes you suggested (OS = "Darwin" and passing the darwin PCSC framework individually) in this PR.

With regard to the formatting changes, I've put them on a separate branch: jerith666/nixpkgs@globalprotect-vpn...jerith666:globalprotect-vpn-formatting. Is there a style guide you're following that led you to these changes, which I could keep in mind for future work? Or are they more of your personal preference?

Is there a style guide you're following that led you to these changes, which I could keep in mind for future work? Or are they more of your personal preference?

Not really. Normally things are defined in the order they are used at build time.

Also please fix the eval issue.

Whoops, missed the eval issue! (In fact there was a second one lurking behind the first ...) 🤦 Fixed now.

I've left the formatting changes on their own branch. IMO the code reads more clearly without them, but it's obviously a matter of opinion and I'm totally fine if you'd like to merge them (or probably better, squash-merge them). :)

@jerith666 I am not sure why this causes so many rebuilds but please target staging.

@ofborg eval

I was also surprised that this ended up rebuilding a bunch of stuff for me locally. One that I remember being surprised about is gnucash. So I used nix-store -q --graph to figure it out:

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep 'openconnect.*->'
"/nix/store/2p3w750xzn398kgl3hy0almaxf6i1w5f-openconnect-8.10.drv" -> "/nix/store/4hp0c8rhn57w7ylr8i61nasgfky3h75b-fix-paths.patch.drv" [color = "magenta"];
"/nix/store/bpgwzg2lb1id41v3kjlj0rid4zw86plb-openconnect-8.10.tar.gz.drv" -> "/nix/store/2p3w750xzn398kgl3hy0almaxf6i1w5f-openconnect-8.10.drv" [color = "green"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep '4hp0c.*->'
"/nix/store/4hp0c8rhn57w7ylr8i61nasgfky3h75b-fix-paths.patch.drv" -> "/nix/store/prv3bchmk48rvhfflznd496x9k2lbjkm-network-manager-1.26.0.drv" [color = "burlywood"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep 'prv3b.*->'
"/nix/store/prv3bchmk48rvhfflznd496x9k2lbjkm-network-manager-1.26.0.drv" -> "/nix/store/swgcbiq4rd9n69k6v4f17w0yv73mnkpp-libproxy-0.4.15.drv" [color = "blue"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep 'swgcb.*->'
"/nix/store/swgcbiq4rd9n69k6v4f17w0yv73mnkpp-libproxy-0.4.15.drv" -> "/nix/store/adc29252yixs60q7c5mjx5qv5sihy290-glib-networking-2.66.0.drv" [color = "red"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep 'adc292.*->'
"/nix/store/adc29252yixs60q7c5mjx5qv5sihy290-glib-networking-2.66.0.drv" -> "/nix/store/12q49n28niihvdcsc37z11llwp97cds7-geoclue-2.5.6.drv" [color = "burlywood"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep '12q49n.*->'
"/nix/store/12q49n28niihvdcsc37z11llwp97cds7-geoclue-2.5.6.drv" -> "/nix/store/7rnb1lz0aybng93lb6mpvsw7vbfhf2df-webkitgtk-2.30.1.drv" [color = "red"];

$ nix-store -q --graph $(nix-store -qd $(type -p gnucash)) | grep '7rnb1l.*->'
"/nix/store/7rnb1lz0aybng93lb6mpvsw7vbfhf2df-webkitgtk-2.30.1.drv" -> "/nix/store/3b1p0n2vi651q1nf510va24j5cqgla3q-gnucash-4.2.drv" [color = "red"];

Nothing in that chain seems obviously wrong to me, so I'll re-target to staging.

Just pinging this, I think all the requested changes have been made and it's okay to merge?

Merge remote-tracking branch 'origin/nixos-unstable' into globalprotect-vpn

We don't really allow merging PRs that contain merge commits.

Please rebase.

@veprbl Good spotting, thanks.

@jerith666 Ready for another (hopefully the last) round😄? Could you remove the merge commit and rebase it onto master?

Rebased without the merge commit. The only outstanding issue is the LANG in the systemd service definition, I believe.

Rebased without the merge commit. The only outstanding issue is the LANG in the systemd service definition, I believe.

Have you tested the service without it?

Tested without the LANG setting, it works; new version force-pushed just now. Maybe it's finally ready? 🤞 😄

@jerith666 Congrats, you did it😃!

Awesome, thanks everyone! Hopefully my machine won't be too bored now that all these rebuilds will be cached ... ;-)

The NixOS module is missing mkIf cfg.enable, so it is always enabled.

Gack -- whoops, good catch! Fix is in #128473.