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 'cargo creusot setup' to manage creusot installations #961

Merged
merged 16 commits into from
Mar 9, 2024

Conversation

Armael
Copy link
Contributor

@Armael Armael commented Mar 2, 2024

The implementation follows the design outlined here: #946 .
Note that this the first time I'm writing a non-trivial amount of Rust code, so I'm happy to take any comments regarding code style, organization, etc.

cargo creusot setup comes with the following subcommands:

  • status: displays the state of the current installation
  • install: sets up creusot in "managed" mode, downloading solver binaries at known-good versions and configuring why3 to use them. (Can also be invoked on an existing creusot installation to upgrade out-of-date solvers.)
  • install-external: minimal setup of creusot in "external" mode, that assumes that the user manages the external tools (why3, solvers, etc) themselves.

For now, even the "managed mode" setup assumes that why3 and alt-ergo have been installed separately (e.g. using opam), but the goal is to later have 'creusot setup' download binaries when we have them.

@Armael
Copy link
Contributor Author

Armael commented Mar 2, 2024

Since this changes a bit the CLI workflow (now you have to go through the setup phase even if it's just for install-external) I probably need to update the CI script and the README. Will do later.
(Regarding the CI, I've used the solver versions that the CI currently uses, and the idea is that the much of the CI script could be replaced by a cargo creusot setup install invocation).

Comment on lines 52 to 55
let s = match fs::read_to_string(p) {
Err(e) => return Err(Error::Invalid(e.to_string())),
Ok(s) => s,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want more ergonomic error handling here we could look at eyre or more generally return a Box<dyn Error> object as the error case. Of you could define an instance of From for the appropriate types.

It doesn't matter much here, but in case you wanted comments for future changes.

Copy link
Collaborator

@xldenis xldenis Mar 2, 2024

Choose a reason for hiding this comment

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

Ah I see you're using anyhow down below. The general approach seems to be thiserror in libraries and eyre (improved version of anyhow) in applications.

The implementation follows the design outlined here:
creusot-rs#946

'cargo creusot setup' comes with the following subcommands:
- 'status': displays the state of the current installation
- 'install': sets up creusot in "managed" mode, downloading solver
  binaries at known-good versions and configuring why3 to use
  them. (Can also be invoked on an existing creusot installation to
  upgrade out-of-date solvers.)
- 'install-external': minimal setup of creusot in "external" mode,
  that assumes that the user manages the external tools (why3,
  solvers, etc) themselves.

For now, even the "managed mode" setup assumes that why3 and alt-ergo
have been installed separately (e.g. using opam), but the goal is to
later have 'creusot setup' download binaries when we have them.
@Armael Armael force-pushed the setup-cmd branch 3 times, most recently from 2c9e3fc to 820f3cd Compare March 8, 2024 20:02
- either use a .creusot-config directory from the root of the git
  repository, if it exists
- or load the gloal creusot config directory (as setup by 'cargo
  creusot setup')
(this is what the CI uses currently)
@Armael Armael force-pushed the setup-cmd branch 2 times, most recently from 6f2b732 to 5fddc1b Compare March 8, 2024 21:56
as a bonus, cache solver binaries using this file as cache key
…h to a library

and also provide a why3 wrapper binary to use in shell scripts
HACKING.md Outdated Show resolved Hide resolved
@Armael
Copy link
Contributor Author

Armael commented Mar 9, 2024

I've updated the PR. There is quite a bit of new code: I've been working on the developer workflow (for people hacking on creusot, running the testsuite, etc) and tried to harmonize it with the "user" workflow introduced by cargo creusot setup.

The idea is that someone who has a working creusot setup (using cargo creusot setup) should be able to start hacking on creusot and use it as configuration also for the testsuite. And alternatively it should still be possible to use a custom why3 or solver when needed.
The solution I propose (and implement here) is documented in HACKING.md: by default the testsuite will use the global config from cargo creusot setup, but if you want to do something different, you can have a .creusot-config directory at the root of the git repository, and that will be used instead (it is ignored by git). (A sample configuration is provided in .creusot-config.sample.)

I've also updated the user setup instructions in README.md to use the new cargo creusot setup install command.

All in all this is a bit more logic/code than I initially expected, but this is what I came up with trying to make things more robust.

HACKING.md Show resolved Hide resolved
@xldenis
Copy link
Collaborator

xldenis commented Mar 9, 2024

The solution I propose (and implement here) is documented in HACKING.md: by default the testsuite will use the global config from cargo creusot setup, but if you want to do something different, you can have a .creusot-config directory at the root of the git repository, and that will be used instead (it is ignored by git). (A sample configuration is provided in .creusot-config.sample.)

I think this is a good workflow, most of the time we are not working against custom solver versions or custom why3 versions. I also think it drastically simplifies the new contributor experience which is valuable as well.

@xldenis
Copy link
Collaborator

xldenis commented Mar 9, 2024

I've been reading over the code today, it all looks really good to me, and I can't wait to try it out.

@Armael Armael merged commit 515703d into creusot-rs:master Mar 9, 2024
4 checks passed
@xldenis
Copy link
Collaborator

xldenis commented Mar 9, 2024

🎉

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.

2 participants