Skip to content

Allow finding and linking system deps#211

Merged
thomwiggers merged 2 commits intoopen-quantum-safe:mainfrom
tranzystorekk:link-system-deps
Sep 13, 2023
Merged

Allow finding and linking system deps#211
thomwiggers merged 2 commits intoopen-quantum-safe:mainfrom
tranzystorekk:link-system-deps

Conversation

@tranzystorekk
Copy link
Copy Markdown
Contributor

Addresses #190

Adds necessary changes to optionally detect the system liboqs version and link to it.

To consider:

  • Should there be a vendor feature, or can we defer to pkg-config crate's env var configuration (e.g. "LIBOQS_NO_PKG_CONFIG")
  • Is targeting the minor version (>= 0.8.0 && < 0.9.0) enough?

Tested this manually with a system-installed build of liboqs 0.8.0-rc1. Not sure how best to test this in CI yet.

@tranzystorekk
Copy link
Copy Markdown
Contributor Author

Oops, meant for this to be a draft for now.

@tranzystorekk
Copy link
Copy Markdown
Contributor Author

Ping @wucke13 for discussion

Copy link
Copy Markdown
Contributor

@thomwiggers thomwiggers left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty reasonable, but I have a few questions/suggestions

Comment thread oqs-sys/build.rs Outdated
Comment thread oqs-sys/build.rs Outdated
@tranzystorekk
Copy link
Copy Markdown
Contributor Author

Added requested changes, split the code into some more includedir related functions to allow early return if vendored is enabled.

Copy link
Copy Markdown
Contributor

@thomwiggers thomwiggers left a comment

Choose a reason for hiding this comment

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

I have a minor gripe with the lower bound computation, and it would be great if you could document the vendored feature in the README.

Comment thread oqs-sys/build.rs Outdated
@tranzystorekk
Copy link
Copy Markdown
Contributor Author

Added the suggested change and README docs. I was split between processing the patch version as an interger or as a str, but the latter seems more reliable, especially against versions like 0.8.001

Copy link
Copy Markdown
Contributor

@thomwiggers thomwiggers left a comment

Choose a reason for hiding this comment

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

Unfortunately, Cargo won't allow 001 and will simplify that to 1; otherwise I would have released version 000. But this seems good enough.

@thomwiggers thomwiggers merged commit cfb27ea into open-quantum-safe:main Sep 13, 2023
@tranzystorekk tranzystorekk deleted the link-system-deps branch September 13, 2023 12:38
@thomwiggers
Copy link
Copy Markdown
Contributor

Shoot, I hadn't thought about this but 001 being not allowed also means that #192 is again unsolved.

@thomwiggers
Copy link
Copy Markdown
Contributor

And that is a problem for releasing updates :(

@tranzystorekk
Copy link
Copy Markdown
Contributor Author

tranzystorekk commented Sep 13, 2023

I guess I also forgot something - an env var like LIBOQS_NO_VENDOR that would make it easier for distributions to ensure we're only built if system package is available.

Such a thing makes it easier for distros to manage a central liboqs package with e.g. sexurity vulnerabilities patched, as with SSL libraries.

@thomwiggers
Copy link
Copy Markdown
Contributor

Sounds like a good addition

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.

2 participants