-
Notifications
You must be signed in to change notification settings - Fork 406
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
crate_universe
now renders json formatted lockfiles
#692
Conversation
This lack of 4.0 is really driving me insane....
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this forwards!
generate_dependencies(config, &opt) | ||
} | ||
|
||
fn reuse_lockfile(config: Config, lockfile: &Path, opt: &Opt) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This structure in general doesn't quite look like I was imagining it would, but I think that's because this is an incremental step which will enable future change?
I'm imagining fn main()
ending up looking like:
// ...
let resolver = config.preprocess()?;
let lockfile_caching_resolver = LockfileCachingResolver::new(resolver, &opt.lockfile);
let consolidator = match lockfile_caching_resolver.resolve(&lockfile) {
Ok(consolidator) => consolidator,
Err(ResolverError::OutOfDateLockFile) => {
// Error about lockfile being out of date
},
Err(err) => { return Err(err); },
};
let renderer = consolidator.consolidate()?;
// ...
The idea being that if we're using a lockfile, we'll treat that as a cache of the Resolver::resolve
function and unconditionally perform consolidation and rendering exactly the same as if we'd missed the cache because no lockfile was in use.
That would also allow us to remove everything except toml
and resolver_config
from Resolver
, and accordingly from the lockfile hash, and would allow the lockfile to just contain the hash, resolved_packages
and probably member_packages_version_mapping
.
That's a little different from your current structure which puts a conditional around everything after Config
parsing, stores effectively a whole Renderer
in the lockfile, and accordingly still has to keep lots of things factored into the hash.
Do we have the same destination in mind, or were you thinking something different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're describing sounds like a good approach to me. I ended up doing this because I thought it was the simpler change and the fastest path to getting well formed lockfiles. Most of the time on this PR was spent trying to figure out what was going on so definitely a more incremental change than a strong directional one.
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - would appreciate applying my last suggestion before merging, if you don't mind!
Notable changes in this PR: - `crate_universe` `lockfile` attributes are now rendered as json files instead of starlark dumps - The `cargo-raze` dependenciy has been updated to a pin which implements the required sorting and de-serialization. - Formatted `use` declarations - Updated dependencies - `CARGO` and `RUSTC` environment variables are now ignored in determining the digest. This should be safe given that `cargo version` is an input to generating the lockfile hash. - `repository_template` has been renamed to `crate_registry_template`, which is more correct. - The [json](https://docs.bazel.build/versions/master/skylark/lib/json.html) module is now used to handle generating the config `.json` file
Notable changes in this PR:
crate_universe
lockfile
attributes are now rendered as json files instead of starlark dumpscrate_universe_resolver
now accepts arepository_dir
argument for the path to the new repository. This should be preferable to a single output file for all dependencies.cargo-raze
dependenciy has been updated to a pin which implements the required sorting and de-serialization.use
declarationsCARGO
andRUSTC
environment variables are now ignored in determining the digest. This should be safe given thatcargo version
is an input to generating the lockfile hash.repository_template
has been renamed tocrate_registry_template
, which is more correct..json
file