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] ./pants lock uses transitive closure as inputs #12312

Merged
merged 3 commits into from
Jul 9, 2021

Conversation

Eric-Arellano
Copy link
Contributor

These are not the proposed final semantics for the ./pants lock goal. The semantics will change when we have per-tool lockfiles and multiple lockfiles, likely such that ./pants lock :: will regenerate all distinct lockfiles.

For now, this PR merely aligns ./pants lock with the current generate_lockfile.sh script, so that we can run ./pants --tag=-lockfile_ignore lock ::. The goal is to replace generate_lockfile.sh with this goal.

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

[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor Author

FYI @chrisjrn

Comment on lines 85 to 89
logger.warning(
"No third-party requirements found for the transitive closure, so a lockfile will not "
"be generated."
)
return LockGoal(exit_code=1)
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to exit=1, this should probably be an error.

Comment on lines +102 to +104
# TODO: Figure out which interpreter constraints to use...Likely get it from the
# transitive closure. When we're doing a single global lockfile, it's fine to do that,
# but we need to figure out how this will work with multiple resolves.
Copy link
Member

Choose a reason for hiding this comment

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

Discussed a bit on #12300 (comment).

For now though, this should likely use the global default interpreter_constraints to be slightly more correct (although obivously not all the way there...)

Comment on lines +71 to +74
# TODO: It's not totally clear to me if that is desirable. Consider requirements like
# pydevd-pycharm. Should that be in the lockfile? I think this needs to be the case when
# we have multiple lockfiles, though: we shouldn't look at the universe in that case,
# only the relevant subset of requirements.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's what you're getting at here, but: the lockfile(s) == "the resolve": if when running/testing or packaging the code we would include a dep, it must be in the lockfile, because when the lockfile is unchanged, we will use the lockfile as the key that identifies an invoke of PEX to build the resolve (which will then be subsetted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pydevd-pycharm is an interesting one because it's not normally used in tests, only when Pants developers add it during debugging. This would mean that they need to regenerate the lockfile every time they add the import. I think that's expected, right?

Copy link
Member

Choose a reason for hiding this comment

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

Hm. Yes, unless perhaps it was "forced" into the resolve by some *_binary/*_test declaration (hand-wave, but you could do it explicitly)? That way it would always be in the resolve, and would just be filtered out by subsetting. Adding it to a test would then not need to re-run the resolve.

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, that makes sense. The bigger thing I'm after is that Pants should only create a lockfile for reqs that are actually used in your project.

Even if you requirements.txt has some-req, if it's never used anywhere, I think we should skip it in the lockfile. And I think that is going to be necessary when we have multiple user lockfiles because it might not be safe to include some of those global, unused reqs in that resolve.

Copy link
Member

@stuhood stuhood Jul 9, 2021

Choose a reason for hiding this comment

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

Even if you requirements.txt has some-req, if it's never used anywhere, I think we should skip it in the lockfile. And I think that is going to be necessary when we have multiple user lockfiles because it might not be safe to include some of those global, unused reqs in that resolve.

Hm. Yes, but ... lockfile == resolve. It's already the case that if you have a requirements.txt file containing unused stuff, the unused stuff will not actually be included in any of the resolves in your repository

... except when "use all constraints" is set. That setting does something different than what a "named resolve" concept should do. Each named resolve contains only the subset of requirements actually used by the roots participating in the resolve.

@Eric-Arellano Eric-Arellano merged commit 1abd7a9 into pantsbuild:main Jul 9, 2021
@Eric-Arellano Eric-Arellano deleted the lock-specs branch July 9, 2021 15:29
illicitonion added a commit to illicitonion/pants that referenced this pull request Jul 9, 2021
Internal changes omitted from the release notes:

* [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312))

* [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300))

* upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308))

* upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294))

* Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302))

* Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279))

* Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287))

* [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274))

* Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273))

* Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922))

* Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267))

* Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261))
illicitonion added a commit to illicitonion/pants that referenced this pull request Jul 9, 2021
Internal changes omitted from the release notes:

* [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312))

* [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300))

* upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308))

* upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294))

* Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302))

* Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279))

* Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287))

* [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274))

* Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273))

* Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922))

* Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267))

* Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261))
illicitonion added a commit to illicitonion/pants that referenced this pull request Jul 9, 2021
Internal changes omitted from the release notes:

* [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312))

* [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300))

* upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308))

* upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294))

* Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302))

* Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279))

* Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287))

* [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274))

* Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273))

* Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922))

* Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267))

* Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261))

* Add JavacSubsystem and JavacBinary, allowing user-configurable JVM selection ([pantsbuild#12283](pantsbuild#12283))

* Revert "Deprecate unused `--process-execution-local-enable-nailgun` (pantsbuild#11600)" ([pantsbuild#12257](pantsbuild#12257))
illicitonion added a commit that referenced this pull request Jul 9, 2021
Internal changes omitted from the release notes:

* [internal] `./pants lock` uses transitive closure as inputs ([#12312](#12312))

* [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([#12300](#12300))

* upgrade tokio and futures crates ([#12308](#12308))

* upgrade tonic, tower, and hyper crates ([#12294](#12294))

* Set up pants to use the latest version of humbug ([#12302](#12302))

* Refactor BUILD file parsing. ([#12279](#12279))

* Use get_type_hints() to get typed type hints. ([#12287](#12287))

* [internal] Fix parameters of convert_source_to_sources_test ([#12274](#12274))

* Fix assert arguments in a test ([#12273](#12273))

* Introduce a native pants client. ([#11922](#11922))

* Don't run CI in forks ([#12267](#12267))

* Change how we embed docsite links in help text. ([#12261](#12261))

* Add JavacSubsystem and JavacBinary, allowing user-configurable JVM selection ([#12283](#12283))

* Revert "Deprecate unused `--process-execution-local-enable-nailgun` (#11600)" ([#12257](#12257))
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