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

Allow windows linking of test programs #1751

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

matlimatli
Copy link
Contributor

The kat_kem and kat_sig programs could not be linked when building natively on windows. This was caused by multiple definitions of symbols. By using the /FORCE:MULTIPLE compiler option, this is allowed, similar in spirit to what was already used for cross-compiling to Windows.

Fixes #1749

  • [No] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [No] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@matlimatli matlimatli requested a review from dstebila as a code owner April 5, 2024 14:05
@matlimatli matlimatli marked this pull request as draft April 5, 2024 14:07
Signed-off-by: Mattias Lindblad <matlin@gmail.com>
@matlimatli matlimatli force-pushed the windows_tests_linking branch from 9a017a0 to 6182fdd Compare April 5, 2024 14:11
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.

Excellent! Thanks very much for this contribution @matlimatli !

Note to self and @smcmahonibm: Let's consider adding a CI test for this config if this is more widely used.

@matlimatli matlimatli marked this pull request as ready for review April 5, 2024 14:19
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@baentsch baentsch merged commit 701dea5 into open-quantum-safe:main Apr 7, 2024
43 checks passed
@vsoftco
Copy link
Member

vsoftco commented Apr 7, 2024

@baentsch @SWilson4 @matlimatli I saw this a bit late.. the easiest (imo) fix is to use the CMake flag -D CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=TRUE when compiling for Windows, it takes care of the multiple defined symbols at linking. I use it on my side on a Windows VBox and works well.

@baentsch
Copy link
Member

baentsch commented Apr 7, 2024

Thanks for the suggestion, @vsoftco . I do like the fix in this PR, though, as it doesn't depend on the setting of a cmake flag: Do you also see it like that or is there something "CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS" does "better"?

@vsoftco
Copy link
Member

vsoftco commented Apr 7, 2024

I don't think there's anything that CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=TRUE does better, I just pointed out that CMake now has the option (since I bumped into the exact same issue a while ago). The only think CMAKE may (although I'not sure) do better is ensuring compatibility across different compilers (say that some other Win compiler comes along, not MSVC). But again, totally fine with this PR as long as it fixes it and works.

@matlimatli
Copy link
Contributor Author

@vsoftco I didn't know about that option. Thanks for pointing it out, it might come in handy in some situations!

@baentsch
Copy link
Member

baentsch commented Apr 8, 2024

Worthwhile a documentation PR, e.g., 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.

Windows build with "-DBUILD_SHARED_LIBS=ON"
4 participants