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

Add Nix files and commit Cargo.lock #300

Merged
merged 24 commits into from
Jun 12, 2024
Merged

Conversation

pnmadelaine
Copy link
Contributor

@pnmadelaine pnmadelaine commented Jun 5, 2024

Add a Nix expression to build ml-kem

@pnmadelaine pnmadelaine marked this pull request as ready for review June 6, 2024 11:46
@pnmadelaine
Copy link
Contributor Author

I disabled LTO in the Nix build as it causes the build to fail. Am I missing something, is there a specific toolchain to use? I am using gcc and ld.

@pnmadelaine
Copy link
Contributor Author

using mold instead of ld seems to fix the issue!

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks, generally looks good except for that one change.

libcrux-ml-kem/c/CMakeLists.txt Show resolved Hide resolved
@pnmadelaine
Copy link
Contributor Author

@franziskuskiefer I'll just continue the conversation here as github seems to double all my comments on the code (?)

I think you meant using fetch_package was not right because it required installing benchmark manually? I used FetchContent_Declare instead, is that okay with you?

@pnmadelaine
Copy link
Contributor Author

I was just thinking, we should discuss committing flake.lock.
If it is committed it needs to be updated regularly, otherwise the nix build will quickly fail and the lock file will become useless anyway.
@W95Psp what do you think?

@W95Psp
Copy link
Contributor

W95Psp commented Jun 11, 2024

(quoting myself from cryspen/bertie#118, since it's the same concern)

I see no point in not committing flake.lock, of we don't commit flake.lock, we should drop the nix expressions altogether. The nix job you added will check the nix expressions are up to date as code changes, that's all we need imo. Locked flakes are especially useful to make sure we'll be able to build the project in the future.

@pnmadelaine pnmadelaine marked this pull request as draft June 11, 2024 14:11
@pnmadelaine
Copy link
Contributor Author

converting to draft as this is currently broken after updating

@pnmadelaine pnmadelaine marked this pull request as ready for review June 11, 2024 19:06
@franziskuskiefer franziskuskiefer merged commit 9186708 into cryspen:dev Jun 12, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants