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

[internal] Add generate_all_lockfiles.py to correctly generate default tool lockfiles #12418

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

Eric-Arellano
Copy link
Contributor

The requirements we use for pantsbuild/pants might be different than what makes sense for the default we provide to our users. For example, we currently use two Flake8 plugins that should not be in the default lockfile.

This script allows us to be confident that the default lockfiles we generate are entirely decoupled from our own internal settings.

@Eric-Arellano Eric-Arellano force-pushed the gen-lockfiles-script branch 2 times, most recently from 533564f to 6da9162 Compare July 27, 2021 02:16
…ult tool lockfiles

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano force-pushed the gen-lockfiles-script branch from 6da9162 to b61c07a Compare July 27, 2021 02:20
[ci skip-rust]
[ci skip-build-wheels]

[ci skip-rust]

[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano changed the title WIP [internal] Add generate_all_lockfiles.py to correctly generate default tool lockfiles [internal] Add generate_all_lockfiles.py to correctly generate default tool lockfiles Jul 27, 2021
@Eric-Arellano Eric-Arellano marked this pull request as ready for review July 27, 2021 03:27
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

GitHub's "viewed" checkbox in the top right corner of each file is quite handy for this PR..

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

Will #12418 be the first thing that enforces that these files haven't gone out of sync? Or do we already have a version check that would fail when someone bumped the default_version in this repo without changing the lockfile?

@@ -72,9 +72,10 @@ class PythonProtobufMypyPlugin(PythonToolRequirementsBase):
"pants.backend.codegen.protobuf.python",
"mypy_protobuf_lockfile.txt",
)
default_lockfile_url = git_url(
default_lockfile_path = (
Copy link
Member

Choose a reason for hiding this comment

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

I guess that the default_lockfile_path is intentionally undocumented / not part of the interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's not used by any of our production code, only to facilitate this script. I don't want to require that plugins implement it because they might not need it.

@Eric-Arellano
Copy link
Contributor Author

Will #12418 be the first thing that enforces that these files haven't gone out of sync?

Oh, tricky. Chris's work on #12415 is what will validate lockfiles are not stale for our own internal usage.

We still need to validate though that the default lockfiles we're providing are not stale..I think the way to do that is to add an integration test that runs each of the tools using lockfile=<default>. Or, update our existing integration tests to use that setting. Eventually, <default> will become the default, but in the meantime, we can hardcode it in tests.

Before that lands, we'll have to rely on code review to ensure this is not stale.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member

stuhood commented Jul 27, 2021

Will #12418 be the first thing that enforces that these files haven't gone out of sync?

Oh, tricky. Chris's work on #12415 is what will validate lockfiles are not stale for our own internal usage.

We still need to validate though that the default lockfiles we're providing are not stale.

If the staleness check works by comparing a hash of inputs to a hash encoded inside the lockfile, then it should work for lockfiles loaded as resources just as well as it should for those loaded via options... seems like it should work...?

@Eric-Arellano
Copy link
Contributor Author

If the staleness check works by comparing a hash of inputs to a hash encoded inside the lockfile, then it should work for lockfiles loaded as resources just as well as it should for those loaded via options... seems like it should work...?

Ah to clarify, the issue is that CI never uses the <default> lockfiles currently. Only our new custom ones under 3rdparty/python/lockfiles. We need our CI to check both our custom ones and our <default> ones. I was pondering how we can do that.

@Eric-Arellano Eric-Arellano merged commit 986e8c9 into pantsbuild:main Jul 27, 2021
@Eric-Arellano Eric-Arellano deleted the gen-lockfiles-script branch July 27, 2021 23:59
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