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 [jvm].default_compatible_resolves #13902

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Dec 16, 2021

See #12742.

The idea with default_compatible_resolvse is to let you set the default to >1, whereas default_resolve would force you to only being able to default to one resolve.

While not extremely common, we do suspect there will be some repositories where the majority of your jvm_artifact and java_source targets work with 2+ resolves, and it's only a few targets that diverge.

This should probably be replaced by #13767 eventually.

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

[ci skip-rust]

[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
# 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]
@stuhood
Copy link
Member

stuhood commented Dec 16, 2021

Although, I'm not sure how desirable this actually is? There's some downside to this PR - it adds another option which is a new layer of complexity. If we think it's reasonable that you'd only ever want to default to 1 resolve, this PR is probably a bad idea.

I think that the general trend is going to be toward all target types having the compatible_resolves field, rather than the resolve field (for the reasons discussed yesterday with @benjyw). But since that is potentially challenging to do before #13882 (since test runners would need native support for doing the cross-product, etc), then we should be thinking of what we're doing with these defaults as a temporary solution.

So: if we think that in the meantime it is easiest to just have the single entry default option, and then only add support for multiple default resolves if users request it, that seems fine to me. Alternatively, if we think that we can replace the single-entry option immediately (by having targets with the resolve field use the first entry in the default list, for example...?), then that seems fine too. But either way, having both default_ options at once seems like overkill.

@Eric-Arellano
Copy link
Contributor Author

But either way, having both default_ options at once seems like overkill.

Okay, that's fine with me. Let's just have --default-resolve for now as the simplest approach.

@Eric-Arellano Eric-Arellano deleted the jvm-default-compatible-resolves branch December 16, 2021 21:10
Eric-Arellano added a commit that referenced this pull request Dec 23, 2021
…olve` (#13967)

Closes #12742.

Similar to #13925, this adds a default of `{"python-default": "3rdparty/python/default_lock.txt"}`. It also lets the user set the default resolve via `--experimental-default-resolve`.

We don't add `--default-compatible-resolves`, as decided in #13902.

## Default file name

The resolve must be unambiguous with JVM, hence using `python-default` instead of `default`.

Everyone agreed in Slack that we should use `python` instead of `py`, e.g. `3rdparty/python` over `3rdparty/py`. The clarity is worth it.

It's unclear to me if we want the file to be called `default_lock.txt`. In JVM, it's called `default.lock`. We could do that here too? I only went with `.txt` to make it a bit more clear to users that they can use these lockfiles just like a normal `requirements.txt` - Python has a strong convention for this.

## Deprecation plan: `--enable-resolves`

We want to require using resolves in the future, as we believe it is important for users to have secure and reproducible builds. But we can't switch it on immediately.

At the same time, to make the feature useful, it's really useful for us to be able to set defaults.

To bridge the gap, `--enable-resolves` feature gates everything. When set, we can act how we will in the future, e.g. assuming that a default resolve must always be set. When unset, we use the status quo. In Pants 2.10, we will deprecate not setting this option so that we can make the default in Pants 2.11 be to use resolves.

[ci skip-rust]
[ci skip-build-wheels]
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