Skip to content

Conversation

@AdijeShen
Copy link
Contributor

Solve Issue #1

Changes Made

  • Changed from dynamic linking (liboqs.so/liboqs.dll) to static linking (liboqs.a)
  • Added liboqs as a git submodule in ./liboqs/
  • Updated build instructions for all platforms

Key Benefits

  1. Simplified installation - no need to install liboqs in system directory
  2. More portable - all dependencies contained within project
  3. Consistent builds across platforms

Documentation Updates

  • Preserved all existing API documentation
  • Maintained all examples and usage instructions
  • Kept original diagrams and status badges
  • Updated only the build/installation sections

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.

Thanks for this PR, @AdijeShen -- before going into review detail, I've asked the rest of the team as to whether they're OK with the submodule approach. The change I'd immediate question is the removal of RELEASE.md: If there's no good reason for this (?) please bring it back: As and if someone else starts to maintain this sub project again, there may be an interest to do new releases again...

@AdijeShen
Copy link
Contributor Author

AdijeShen commented Nov 1, 2024 via email

@SWilson4
Copy link
Member

Hi @AdijeShen, do you plan to continue with this PR?

@AdijeShen
Copy link
Contributor Author

Hi @AdijeShen, do you plan to continue with this PR?

Hi @SWilson4 , I will turn back to working on this PR and plan to finish it within one week.

@AdijeShen
Copy link
Contributor Author

I've completed my revisions by removing the git submodule approach as requested. The current implementation uses static linking with liboqs.a instead of dynamic linking, which might require adjustments to the GitHub CI workflow.

Please let me know if any further changes are needed. I'm available to address additional feedback to get this PR ready for merging.

@AdijeShen AdijeShen requested a review from baentsch March 20, 2025 11:30
@SWilson4 SWilson4 dismissed baentsch’s stale review March 21, 2025 14:07

submodule removed

@SWilson4
Copy link
Member

I've completed my revisions by removing the git submodule approach as requested. The current implementation uses static linking with liboqs.a instead of dynamic linking, which might require adjustments to the GitHub CI workflow.

Please let me know if any further changes are needed. I'm available to address additional feedback to get this PR ready for merging.

Thanks for the updates! I am not at all knowledgeable about Windows development and build processes (especially in Java), so please bear with me. Could you elaborate on why it's necessary to change from dynamic to static linking? It would also be great if you could add a CI workflow for Windows.

You will also need to add DCO signoff to your commits. Instructions for that are here: https://github.com/open-quantum-safe/liboqs-java/pull/28/checks?check_run_id=39103331593.

AdijeShen and others added 16 commits March 23, 2025 09:08
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Huajie Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Huajie Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
* Port workflows from CircleCI to GitHub Actions
* Add CI job for workflow linting
* Update README badge

---------

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
…fe#32)

* Update to latest CI image

Signed-off-by: Spencer Wilson <[email protected]>

* Install jdk package and set JAVA_HOME

Signed-off-by: Spencer Wilson <[email protected]>

* Add macOS job

Signed-off-by: Spencer Wilson <[email protected]>

* Update README.md and example files

Signed-off-by: Spencer Wilson <[email protected]>

* Remove unnecessary install

Signed-off-by: Spencer Wilson <[email protected]>

---------

Signed-off-by: Spencer Wilson <[email protected]>
* Support derandomized key generation for ML-KEM

Signed-off-by: Spencer Wilson <[email protected]>

* Update example output in README.md

Signed-off-by: Spencer Wilson <[email protected]>

---------

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Adije Shen <[email protected]>
Signed-off-by: adijeshen <[email protected]>
Signed-off-by: adijeshen <[email protected]>
Signed-off-by: adijeshen <[email protected]>
@AdijeShen
Copy link
Contributor Author

Hi @SWilson4, Thank you for the feedback!

Regarding the change from dynamic to static linking:

  1. Static linking resolves Windows-specific issues where loading dynamic libraries requires specific path configurations
  2. It simplifies cross-platform deployment since users don't need to manage separate .dll/.so files
  3. It improves portability by bundling all dependencies within the project

I've added a Windows CI workflow in my latest commits (42f62eb) to validate the build process on Windows environments. The workflow tests the compilation and basic functionality to ensure compatibility. All actions passed in my forked repo

Let me know if you'd like me to make any additional changes or provide more details about the implementation.

@AdijeShen AdijeShen requested a review from SWilson4 March 25, 2025 10:46
@SWilson4
Copy link
Member

SWilson4 commented Apr 1, 2025

Hi @AdijeShen, it looks like the whole repo has been reformatted to use CRLF line endings (\r\n). Could you please reformat to use only LF (\n)?

@AdijeShen
Copy link
Contributor Author

@SWilson4 Thank you for pointing that out. I've fixed the line ending issue by converting all files back to LF format. The changes have been pushed in the latest commit.

@dstebila
Copy link
Member

Thanks very much. Can you add the DCO sign-off? See instructions at https://github.com/open-quantum-safe/liboqs-java/pull/28/checks?check_run_id=39813683997

Signed-off-by: root <root@adijethinkbook.>
Signed-off-by: Adije Shen <[email protected]>
Copy link
Contributor Author

@AdijeShen AdijeShen left a comment

Choose a reason for hiding this comment

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

add signoff

@dstebila
Copy link
Member

@SWilson4 any further thoughts? Otherwise I think we can merge.

@SWilson4
Copy link
Member

@johngray-dev As one of the primary users of the Java wrapper, could you please take a look at this PR? I'd like to understand the implications of moving from dynamic to static linking before we proceed.

Signed-off-by: Spencer Wilson <[email protected]>
@SWilson4
Copy link
Member

In the absence of any objections, merging once re-triggered CI passes. Thanks for the contribution @AdijeShen!

@SWilson4 SWilson4 merged commit 97353bc into open-quantum-safe:master Jun 13, 2025
6 checks passed
@jimouris
Copy link
Member

Should we close #1?

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.

6 participants