-
Notifications
You must be signed in to change notification settings - Fork 115
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
Missing definition of OQS_init() and OQS_destroy() breaks compilation #152
Comments
This error can only happen if
I'd agree. @bhess: Would you want to reconsider how to do this? Having a submodule isn't my first choice given how this can trip up people. If we'd go that way, I'd also suggest adding |
I'd rather use submodules than documenting in the README, also for the liboqs dependency. This way we can also avoid issues like reported here when using outdated dependencies. |
In my case I already have liboqs installed system-wide for user by different packages, and I don't need or want oqs-provider to clone any build its own copy. No thanks. I'd rather quit using this package than let it define liboqs as a submodule. Next you'd want to pull the whole OpenSSL as a submodule too? While I don't use qsc separately (so can't use the same argument), surprising error with cmake failing to git-clone it (while "standalone" git has no problem cloning this repo!) makes me very concerned. A user who's smart enough to clone oqs-provider from GitHub in the first place, is likely smart enough to clone qsc-encoder as well, especially if the Build section of README tells him to. If you aren't sure - just try them. ;) UpdateWhen adding code patch that depends on a certain version of a certain library - the one sane requirement is to explicitly document that at least in the comments of the offending PR. And not doing this - is bad. |
@mouse07410 Good argument that someone may not want to copy something (again) already present (for other purposes). We certainly never will pull @bhess Given the rather "unspectacular" reception of QSC encoding what about changing it to an optional component (not pulled by default but only "by documentation"/sample build scripts)? At least the IETF interop community stated pretty clearly they don't think this encoding is needed. If it is, let's reconsider, but for the time being, it's just causing head-scratching (also in this issue)... |
Point taken @mouse07410. And yes @baentsch, we can make |
Could you please report the cmake version you are using? |
Naturally: $ cmake --version
cmake version 3.24.4
CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ port installed cmake
The following ports are currently installed:
cmake @3.24.4_0 (active)
$ The funny part is that in some cases(when building |
The only reference I find about segfaulting 'git clone' is related to older libcurl versions in git: https://bugzilla.redhat.com/show_bug.cgi?id=1873327 Running |
I can't explain it, but this time it did not seem to even need to pull... |
If oqs-provider is updated after @baentsch's PR #153, this is expected (sets |
No, I changed this default for this test. Though in general I intend to keep that default for "production" builds. |
OK, the main point of this issue has been addressed. PR should be amended with a comment regarding what version(s) of liboqs it applies to, and a warning that it breaks the provider is an older liboqs is used. |
Describe the bug
Current master fails to build - commit fbd2538 is at fault.
Also, fails to clone qsc-key-encoder.git
To Reproduce
Steps to reproduce the behavior:
git clone https://github.com/open-quantum-safe/oqs-provider.git --recurse-submodules
cmake -DCMAKE_BUILD_TYPE=Debug -DOPENSSL_ROOT_DIR="$HOME/src/openssl" -DCMAKE_C_FLAGS="$CFLAGS -I/opt/local/include -L/opt/local/lib" -DCMAKE_VERBOSE_MAKEFILE:BOOL=True -S . -B _build
cmake --build _build
Expected behavior
A normal clean build, like before.
Environment (please complete the following information):
Additional context
IMHO, it's not really nice to git-clone other repositories in the mid-build. Let's either define them as submodules, or add as a pre-requisite to the README.
The text was updated successfully, but these errors were encountered: