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

openconnect: Add darwin support #71318

Merged
merged 2 commits into from Nov 8, 2019
Merged

Conversation

tricktron
Copy link
Member

@tricktron tricktron commented Oct 18, 2019

Motivation for this change

So far the derivation had no Darwin support. The build was failing on darwin because yubikey needs the PCSC framework dependency so I added it. I also upgraded to the latest vpnc-script as recommended on the openconnect homepage (see: https://www.infradead.org/openconnect/platforms.html) because with the old vpnc-script openconnect did not work properly. Lastly, I moved all dependencies which are not used at runtime from propagatedBuildInputs to buildInputs (Please correct me if that is wrong).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Things to do

Would be nice if someone could test it on NixOS and linux (Maybe @trobert in combination with your pkcs11 support).

Notify maintainers

cc @pradeepchhetri

The old build was failing on darwin because yubikey needs the PCSC framework dependency. I also upgraded to the latest vpnc-script as recommended on the openconnect homepage (see: https://www.infradead.org/openconnect/platforms.html). Lastly, I moved all dependencies which are not used at runtime from propagatedBuildInputs to buildInputs (Please correct me if that is wrong).
@risicle
Copy link
Contributor

risicle commented Oct 20, 2019

Builds for me, macos 10.13.

@tricktron
Copy link
Member Author

Anyone on linux using openconnect who can test this?

@FRidh
Copy link
Member

FRidh commented Oct 22, 2019

@tricktron @trobert could you review each others changes?
#68780

@tricktron
Copy link
Member Author

I successfully managed to build this derivation on linux as well.

@FRidh FRidh added this to Needs review in Staging Oct 28, 2019
@trobert
Copy link
Contributor

trobert commented Oct 28, 2019

I successfully tested these changes combined with those in #68780 on linux.

However I am not sure about the propagatedBuildInputs change. At first I agreed but then I saw that openconnect also provides a lib. So it could make sense to propagate the dependencies for the lib users (like connman-gtk ?).
On the other hand, I think vpnc should not be part of any *buildInputs.
Someone more experienced on Nix than me may be able to help more on this part.

Last, did you also test openconnect_openssl on darwin ? On linux it requires an addtional lib (libp11)

@tricktron
Copy link
Member Author

However I am not sure about the propagatedBuildInputs change.

Me neither. My understanding is that every build time dependency should go into buildInputsand every runtime dependency into propagatedBuildInputs. So you are probably right and every library should stay in the propagatedBuildInputs.

Someone more experienced on Nix than me may be able to help more on this part.

Exactly. Maybe @FRidh knows more?

Last, did you also test openconnect_openssl on darwin ? On linux it requires an addtional lib (libp11)

Yes I tested it and it builds successfully. Openssl does not require libp11. Only pkcs11 requires libp11 support if using openssl. So without libp11 -> no pkcs11 support using openssl. With libp11 -> pkcs11 support using openssl.

@trobert
Copy link
Contributor

trobert commented Oct 29, 2019

after a second look, I think the vpnc script need also to be patched to use /nix/store paths for its dependencies (ip, netstat, awk, ...), as in the original vpnc derivation.


assert (openssl != null) == (gnutls == null);

stdenv.mkDerivation rec {
let vpnc = fetchgit {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this different from the vpnc package? If possible it would be better to use vpnc.src instead so it gets updated consistently.

Copy link
Contributor

Choose a reason for hiding this comment

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

openconnect recommend this version over the script in the original vpnc package:

this updated version [...] has support for IPv6, and for running on Solaris and on newer Linux kernels amongst other bug fixes.

(ref openconnect website)

pkgs/tools/networking/openconnect/default.nix Outdated Show resolved Hide resolved
Staging automation moved this from Needs review to Ready Nov 8, 2019
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

Thanks!

@LnL7 LnL7 merged commit 7fb0d95 into NixOS:master Nov 8, 2019
Staging automation moved this from Ready to Done Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants