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.

Sign up for GitHub

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

Jump to bottom

Fix OQS_ADDL_SOCKET_LIBS setting for cmake #256

Closed
wants to merge 2 commits into from
Closed

Fix OQS_ADDL_SOCKET_LIBS setting for cmake #256

wants to merge 2 commits into from

Conversation

qnfm
Copy link
Contributor

@qnfm qnfm commented Sep 17, 2023

This fixes the cmake flag so it generates the right lib dependencies for msvc build. Otherwise, ths VS would complain about:
LINK : fatal error LNK1104: cannot open file 'ws2_32.lib gdi32.lib crypt32.lib'

@qnfm qnfm requested a review from baentsch as a code owner September 17, 2023 16:58
baentsch
baentsch previously approved these changes Sep 17, 2023
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

This fixes the cmake flag so it generates the right lib dependencies for msvc build. Otherwise, ths VS would complain about: LINK : fatal error LNK1104: cannot open file 'ws2_32.lib gdi32.lib crypt32.lib'

Thanks for this contribution! Just one question: Why did VS CI tests pass even without this PR (separating the libs by " ")? Can you point to some documentation requiring the ";"as per this improvement? I'm approving as I don't have any real experience with VS development and assume you do have more.

@qnfm
Copy link
Contributor Author

qnfm commented Sep 17, 2023 •
edited
Loading

This fixes the cmake flag so it generates the right lib dependencies for msvc build. Otherwise, ths VS would complain about: LINK : fatal error LNK1104: cannot open file 'ws2_32.lib gdi32.lib crypt32.lib'

Thanks for this contribution! Just one question: Why did VS CI tests pass even without this PR (separating the libs by " ")? Can you point to some documentation requiring the ";"as per this improvement? I'm approving as I don't have any real experience with VS development and assume you do have more.

I did search the cmake doc and found out a more conventional way to fix the issues. The problem is probably caused by the string in

set(OQS_ADDL_SOCKET_LIBS "ws2_32.lib gdi32.lib crypt32.lib")

The previous code forced cmake into thinking the three libraries as a whole. The later as below followed the cmake normal syntax (recommended).

set(OQS_ADDL_SOCKET_LIBS ws2_32.lib gdi32.lib crypt32.lib)

About "Why did VS CI tests pass even without this PR":
The workflow defined in windows.yml only covers cygwin and msvc(ninja), I was trying to build with Visual Studio 2019.

cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS="/wd5105" -S . -B _build

@qnfm qnfm requested a review from baentsch September 18, 2023 09:14
@baentsch
Copy link
Member

The workflow defined in windows.yml only covers cygwin and msvc(ninja), I was trying to build with Visual Studio 2019.

Understood. What about adding a CI job for this then as part of this PR, too? This way we can be sure this never happens again.

@qnfm
Copy link
Contributor Author

qnfm commented Sep 20, 2023 •
edited
Loading

The workflow defined in windows.yml only covers cygwin and msvc(ninja), I was trying to build with Visual Studio 2019.

Understood. What about adding a CI job for this then as part of this PR, too? This way we can be sure this never happens again.

I tried to add a CI job but encountered a several problems:

Issues 1:

If liboqs is built with Windows 2019 (shipped only with latest Visual Studio version 16), it would failed and the CI raw output as below:

2023-09-20T08:39:22.8287458Z   aes_c.c
2023-09-20T08:39:22.9203546Z   sha2.c
2023-09-20T08:39:22.9957245Z   sha2_c.c
2023-09-20T08:39:23.1140556Z   xkcp_sha3.c
2023-09-20T08:39:23.2030047Z   xkcp_sha3x4.c
2023-09-20T08:39:23.2957625Z   common.c
2023-09-20T08:39:27.6461796Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(487,17): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6463896Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(502,17): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6464982Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(530,17): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6466142Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(531,13): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6466961Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(533,9): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6467730Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(534,5): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6468879Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(665,16): error C2079: 'varDefaultValue' uses undefined struct 'tagVARIANT' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6470079Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(950,16): error C2079: 'varValue' uses undefined struct 'tagVARIANT' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.6977220Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(319,24): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7351959Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(378,37): error C2371: 'pvarVal': redefinition; different basic types [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7360909Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(510): message : see declaration of 'pvarVal' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7365515Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(379,9): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7366633Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(380,5): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7367594Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(383,3): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:27.7368566Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(384,1): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:28.6508799Z   nistseedexpander.c
2023-09-20T08:39:28.7159898Z   fips202.c
2023-09-20T08:39:28.7476426Z   fips202x4.c
2023-09-20T08:39:28.8197615Z   rand.c
2023-09-20T08:39:29.4582529Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(487,17): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4583401Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(502,17): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4586369Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(530,17): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4587171Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(531,13): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4589071Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(533,9): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4591199Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(534,5): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4594499Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(665,16): error C2079: 'varDefaultValue' uses undefined struct 'tagVARIANT' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4600091Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(950,16): error C2079: 'varValue' uses undefined struct 'tagVARIANT' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4733958Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(319,24): error C2059: syntax error: '/' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4737829Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(378,37): error C2371: 'pvarVal': redefinition; different basic types [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4739102Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\oaidl.h(510): message : see declaration of 'pvarVal' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4740070Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(379,9): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4742343Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(380,5): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4743383Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(383,3): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.4745466Z C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\um\propidlbase.h(384,1): error C2059: syntax error: '}' [D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj]
2023-09-20T08:39:29.5852296Z   rand_nist.c
2023-09-20T08:39:29.6802712Z   Generating Code...
2023-09-20T08:39:29.9024228Z Done Building Project "D:\a\oqs-provider\oqs-provider\liboqs\build\src\common\common.vcxproj" (default targets) -- FAILED.

Issues 2:

Change to Visual Studio version 16/MSVC 19 solved the first issue. The liboqs pass the build but if I run the follow test code:

cmake --build build --target run_tests

All the tests failed. But I also solved the issue 2.

Issue 3:

When CI run tests for oqs-provider, CTest reports no tests found without -C on multi-config generators.

Original command

ctest -V --test-dir _build

As cmake issue point out, we need to add args "-C Debug" or "-C Release" accordingly in the build tree with a VS generator.

Issue 4:

Even if I called "ctest -V --test-dir _build", ctest still cannot pass.

Conclusion:
Not final solution
Most of the case is due to file not found. (I didn't fix all the test code yet, so I am not sure whether there is any other problems). I might open a issue in liboqs and make a pr for part of the fix.

@qnfm qnfm mentioned this pull request Sep 21, 2023
Closed
@baentsch
Copy link
Member

Conclusion:
Not final solution

Please let me know when I should take another look.

@baentsch baentsch dismissed their stale review September 22, 2023 12:24

b/o "Not final solution"

@qnfm qnfm mentioned this pull request Sep 22, 2023
Merged
@baentsch
Copy link
Member

Superceded/replaced by #263

@baentsch baentsch closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch Awaiting requested review from baentsch baentsch is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qnfm @baentsch