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] DRY rule to generate a lockfile #12378

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

Eric-Arellano
Copy link
Contributor

Soon, our lockfile generation will get much fancier to generate one lockfile per valid Python interpreter version, then merge into a single lockfile.

This is prework to use a consistent rule to generate a lockfile.

[ci skip-rust]
[ci skip-build-wheels]

# 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]
req: PythonLockfileRequest, pip_tools_subsystem: PipToolsSubsystem
) -> PythonLockfile:
input_requirements = await Get(
Digest, CreateDigest([FileContent("reqs.txt", "\n".join(req.requirements).encode())])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this file name will be included in the generated lockfile like this:

black==21.5b2 \
--hash=sha256:1fc0e0a2c8ae7d269dfcf0c60a89afa299664f3e811395d40b1922dff8f854b5 \
--hash=sha256:e5cf21ebdffc7a9b29d73912b6a6a9a4df4ce70220d523c21647da2eae0751ef
# via -r requirements_black.in

This is somewhat of a UX regression to always use reqs.txt, but I figure it might in some ways be less confusing than the old naming schemes we were using? Thoughts on an optimal UX? I'd rather prioritize good UX than simpler code here.

Note that we'll be changing the lockfile header already to share information about how the lockfile can be regenerated, along w/ maybe things like which ICs were used.

#
# This file is autogenerated by pip-compile with python 3.6
# To update, run:
#
# pip-compile --allow-unsafe --generate-hashes --output-file=src/python/pants/backend/python/lint/black/lockfile.txt requirements_black.in
#

Comment on lines -289 to +257
logger.info(f"Wrote lockfile for {result.tool_name} to {result.path}")
logger.info(f"Wrote lockfile to {result.path}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a UX regression imo. I couldn't figure out a good, generic way to store this info on PythonLockfile because a tool_name property no longer makes sense.

Thoughts if this is worth trying to fix? It might be clear enough already what's happening.

Copy link
Member

Choose a reason for hiding this comment

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

I think that this is the slot for the "resolve name" that I've been referring to.

User resolves will need to be identified by something when specified on pex_binary, etc, and identifying them by the full lockfile name seems a bit boilerplatey. Since we're not going with targets for resolves, that probably leaves us with choosing a slug for the resolve and allowing its path to be configured in pants.toml, with a default "default" resolve in the list automatically. See previous comment on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, cool. I'll add a TODO in a followup to restore this once we have multiple user 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]
Comment on lines -289 to +257
logger.info(f"Wrote lockfile for {result.tool_name} to {result.path}")
logger.info(f"Wrote lockfile to {result.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 think that this is the slot for the "resolve name" that I've been referring to.

User resolves will need to be identified by something when specified on pex_binary, etc, and identifying them by the full lockfile name seems a bit boilerplatey. Since we're not going with targets for resolves, that probably leaves us with choosing a slug for the resolve and allowing its path to be configured in pants.toml, with a default "default" resolve in the list automatically. See previous comment on this.

@Eric-Arellano Eric-Arellano merged commit e01e353 into pantsbuild:main Jul 20, 2021
@Eric-Arellano Eric-Arellano deleted the dry-gen branch July 20, 2021 17:11
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