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

Added /usr/local/include/ykpiv to CGO include path for Linux #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sybrenstuvel
Copy link

On Ubuntu Linux (and derivatives, like the Mint Linux I'm using) the bundled version of yubico-piv-tool is 1.4.2, and is incompatible with this Go library (also see #16).

This PR makes it possible to build the latest version of yubico-piv-tool and install it to /usr/local. Without this change to the CGO parameters, the headers wouldn't be found in /usr/local/include/ykpiv.

It's still necessary to ensure /usr/local/lib is mentioned in /etc/ld.so.conf for the linker to find the library.

This makes it possible to build the latest version of
[yubico-piv-tool](https://developers.yubico.com/yubico-piv-tool/) and
install it to `/usr/local`. Without this change to the CGO parameters,
the headers wouldn't be found in `/usr/local/include/ykpiv`.

It's still necessary to ensure `/usr/local/lib` is mentioned in
`/etc/ld.so.conf` for the linker to find the library.
@sybrenstuvel
Copy link
Author

sybrenstuvel commented May 30, 2019

By the way, an alternative approach would be, instead of adding /usr/include/ykpiv to the include directories, to just use #include <ykpiv/ykpiv.h>. This would let the compiler find the header file in /usr/include as well as /usr/local/include.

This would certainly work on Linux, but I don't know the implications for Windows.

@paultag
Copy link
Member

paultag commented May 30, 2019

Switching to /usr/local will break systems where you've installed it using a system installer. I like the include directories idea better - this change will fix one configuration but break every current user's configuration :)

Maybe we can test on a Windows box - iirc the SDK is dropped in the CWD and you build linking against those. Windows currently uses #cgo windows CFLAGS: -I./win/include/ykpiv/, so we could just change that to be #cgo windows CFLAGS: -I./win/include and fix the imports.

I like that idea much more, any chance we could rework this patch to do that? I'd absolutely merge something doing that. It's also more semantically correct.

@sybrenstuvel
Copy link
Author

I can definitely rework the patch to do that, no problem :)

@paultag
Copy link
Member

paultag commented May 31, 2019

You rock, thank you!

@tianon
Copy link
Collaborator

tianon commented May 31, 2019

https://packages.debian.org/sid/libykpiv-dev comes with pkg-config files (ykcs11.pc and ykpiv.pc) -- are those part of the upstream distribution too, or are those added by the Debian maintainers? (That could be another option to help insulate against the issue, ala // #cgo pkg-config: ...)

@paultag
Copy link
Member

paultag commented May 31, 2019

@tianon amazing idea if so!

@tianon
Copy link
Collaborator

tianon commented May 31, 2019

ykpiv.pc does appear to be part of https://developers.yubico.com/yubico-piv-tool/Releases/yubico-piv-tool-1.7.0.tar.gz, at least! (not sure if that helps Windows, but it should help with /usr/local vs /usr)

@paultag
Copy link
Member

paultag commented May 31, 2019

Yeah I think tianon's right here 👍👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants