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 #118

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Add Nix files and commit Cargo.lock #118

merged 9 commits into from
Jun 12, 2024

Conversation

pnmadelaine
Copy link
Contributor

No description provided.

@pnmadelaine pnmadelaine requested a review from a team as a code owner June 3, 2024 14:23
Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

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

Thanks!
I have a few comments:

  • Why only "x86_64-linux"? Things should work fine on other architectures.
  • We should expose the Rust binaries provided by the crate (e.g. simple_https_{server,client})
  • We should also run hax-driver.py extract-proverif

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.

When committing the log file you need to update the CI here to update dependencies.

@pnmadelaine
Copy link
Contributor Author

@franziskuskiefer this would be a very weird CI workflow, where the lockfile is completely decorrelated from what is actually tested.
Best thing to do imo is, as @W95Psp suggested on Zulip, to use dependabot as a separate workflow to keep dependencies updated.

@franziskuskiefer
Copy link
Member

franziskuskiefer commented Jun 4, 2024

I don't see anything weird.
In any case. This PR needs to go along with a CI here that tests the library without the lock file.
I don't see how dependabot helps with that. But if you have a workflow with that, feel free to propose it.

@pnmadelaine
Copy link
Contributor Author

I don't see anything weird.

Having the lockfile completely ignored by CI, and thus the CI not giving information about the current build status of the repository is at the very least counter-intuitive.

In any case. This PR needs to go along with a CI here that tests the library without the lock file. I don't see how dependabot helps with that. But if you have a workflow with that, feel free to propose it.

The workflow we propose is to run tests with the locked dependencies, so as to ensure the same set of dependencies is used by developers, users and the CI, and to update them periodically with a separate workflow (dependabot, which is already configured on this repository).

@pnmadelaine
Copy link
Contributor Author

I don't see how dependabot helps with that.

dependabot will automatically open PRs to update outdated dependencies

@franziskuskiefer
Copy link
Member

Having the lockfile completely ignored by CI, and thus the CI not giving information about the current build status of the repository is at the very least counter-intuitive.

No one said that it should be ignored. But you must test both cases, using the lock file, and not using it.

so as to ensure the same set of dependencies is used by developers, users and the CI

This is not the case. Cargo doesn't use the lock file in many cases. That's also why we need to test with the toml, as that's the source of truth.

@W95Psp W95Psp mentioned this pull request Jun 6, 2024
Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Looks like there is a linting issue, somehow 👀
Can you rebase your PR with #119? I pushed a fix there

@pnmadelaine
Copy link
Contributor Author

@W95Psp done!

@W95Psp W95Psp dismissed franziskuskiefer’s stale review June 6, 2024 11:59

Changes have been addressed

@W95Psp W95Psp enabled auto-merge June 6, 2024 11:59
@W95Psp W95Psp requested a review from a team June 6, 2024 12:20
@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
Collaborator

W95Psp commented Jun 10, 2024

I see no point in not commiting flake.lock, of we don't commit flake.lock, we should drop the nix expressions all together.
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.

@W95Psp W95Psp merged commit e57d90e into cryspen:main Jun 12, 2024
10 checks passed
@pnmadelaine pnmadelaine deleted the nix branch June 12, 2024 07:48
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.

4 participants