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

V2 lock file format #822

Merged
merged 3 commits into from
Jan 25, 2023
Merged

Conversation

shs96c
Copy link
Collaborator

@shs96c shs96c commented Jan 6, 2023

Introduces (and uses!) a new lock file format. This format is designed to change less as dependencies are updated, which should make reviewing update PRs in GitHub a nicer experience. As a nice side-effect, the new lock files are also (typically) more compact.


exports_files([
"hasher_deploy.jar", # built from //private/tools/java:hasher_deploy.jar
"list_packages_deploy.jar", # build from //private/tools/java/rules/jvm/external/jar:ListPackages_deploy.jar
"hasher_deploy.jar", # built from //private/tools/java/rules/jvm/external:hasher-tool_deploy.jar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know if we can use buildkite to create these jars on each CI build? Then at least we'd have non-local dev builds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. We'd also (presumably) want buildkite to update the PR or fail the build if these change....

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that could work too. Mind adding a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

private/versions.bzl Show resolved Hide resolved
@meisterT
Copy link

Simon, can you given an example for the new lockfile format?

@jin
Copy link
Collaborator

jin commented Jan 12, 2023

@meisterT the diffs contain the newly generated json lockfiles.

@meisterT
Copy link

Thanks Jin, github didn't load it by default and so I missed it.

Is the summary of the new format to reduce nesting?

@shs96c
Copy link
Collaborator Author

shs96c commented Jan 12, 2023

The fundamental goal is to have a lock file format that's easier to review by humans (eg. in PR reviews) A secondary goal is to make the build file more compact, but that's really just a nice benefit.

FWIW, I created a small diff to demonstrate what an update might look like. This is bumping the guava version.

@meisterT
Copy link

The fundamental goal is to have a lock file format that's easier to review by humans (eg. in PR reviews) A secondary goal is to make the build file more compact, but that's really just a nice benefit.

You achieved this goal by avoiding nesting or are there other things to consider?

FWIW, I created a small diff to demonstrate what an update might look like. This is bumping the guava version.

That looks very easy to review indeed!

@shs96c
Copy link
Collaborator Author

shs96c commented Jan 12, 2023

You achieved this goal by avoiding nesting or are there other things to consider?

The existing lock file contains a lot of duplicate information, the main thing being that there are not only the direct dependencies listed in one property, but also all the transitive dependencies, in each and every dependency. By removing that duplication, the file gets a lot smaller (particularly in regular projects --- the examples in the rules_jvm_external repo tend to be tightly focused, so aren't that big).

@cheister
Copy link
Collaborator

LGTM.

small nit: Running bazel run @unpinned_rules_jvm_external_deps//:pin on your branch sorts the repositories differently in rules_jvm_external_deps_install.json

@shs96c shs96c mentioned this pull request Jan 16, 2023
5 tasks
coursier.bzl Show resolved Hide resolved
Copy link
Collaborator

@jin jin left a comment

Choose a reason for hiding this comment

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

lgtm some testing nits

@shs96c
Copy link
Collaborator Author

shs96c commented Jan 18, 2023

I've been asking for comments on the lock file format from people at work, and they suggested that it might be easier for a human to review if the version and shasums were grouped closer to the artifact key in the lock file. I've created some straw-man example updates here.

The first uses the lock file format currently proposed in this PR.

The second uses the format amended to bring shasums closer to the version.

I'd appreciate any input before I continue with the PR....

@jin
Copy link
Collaborator

jin commented Jan 19, 2023

I can definitely see the argument for the new versio. It looks easier to review.

This is designed to be more friendly when making updates,
particularly if the lock files are being reviewed as part
of a PR in GitHub.
@shs96c shs96c merged commit c0436bd into bazel-contrib:master Jan 25, 2023
@shs96c shs96c deleted the v2-lock-file-format branch January 25, 2023 16:34
@alexeagle
Copy link
Contributor

Congrats on shipping :)

shs96c pushed a commit to bazel-contrib/rules_jvm that referenced this pull request Jan 30, 2023
@rdesgroppes
Copy link
Contributor

Nice, thank @shs96c! Would it be possible to expose the conflict_resolution section in a form or another?

@shs96c
Copy link
Collaborator Author

shs96c commented Feb 17, 2023

Can you please file a new issue for that? It should be easy enough to add it.

I'd also very happily accept a PR :)

@rdesgroppes
Copy link
Contributor

Can you please file a new issue for that? It should be easy enough to add it.

I'd also very happily accept a PR :)

Thanks for doing it through #862!

thirtyseven pushed a commit to thirtyseven/rules_jvm_external that referenced this pull request Jun 29, 2023
This is designed to be more friendly when making updates,
particularly if the lock files are being reviewed as part
of a PR in GitHub.
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