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] Better error message for ./pants repl when using multiple resolves #14327

Merged
merged 2 commits into from
Feb 5, 2022

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Feb 1, 2022

First part of #14295. Finishes Python - JVM still needs it.

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

@Eric-Arellano Eric-Arellano deleted the repl-multiple-resolves branch February 1, 2022 21:45
@Eric-Arellano
Copy link
Contributor Author

Blocked by figuring out what the command should be, specifically whether to recommend using JQ vs if we need first-class support for filter --field. Imo, JQ is still better than the status quo!

@stuhood
Copy link
Member

stuhood commented Feb 1, 2022

The JVM should already have a good error message for this: locating a resolve for the roots is a common operation across all of the JVM goal implementations, and the only difference for repl is that it does zero partitioning beforehand.

class NoCompatibleResolve(Exception):
"""No compatible resolve could be found for a set of targets."""
def __init__(self, jvm: JvmSubsystem, msg_prefix: str, incompatible_targets: Iterable[Target]):
targets_and_resolves_str = bullet_list(
f"{t.address.spec}\t{jvm.resolves_for_target(t)}" for t in incompatible_targets
)
super().__init__(
f"{msg_prefix}:\n"
f"{targets_and_resolves_str}\n"
"Targets which will be merged onto the same classpath must have at least one compatible "
f"resolve (from the [resolve]({doc_url('reference-deploy_jar#coderesolvecode')}) or "
f"[compatible_resolves]({doc_url('reference-java_sources#codecompatible_resolvescode')}) "
"fields) in common."
)

@Eric-Arellano
Copy link
Contributor Author

Python already has that same error message :)

# Note: Inspired by `coursier_fetch.py`.
class NoCompatibleResolveException(Exception):
"""No compatible resolve could be found for a set of targets."""
def __init__(
self, python_setup: PythonSetup, msg_prefix: str, relevant_targets: Iterable[Target]
) -> None:
resolves_to_addresses = defaultdict(list)
for tgt in relevant_targets:
if tgt.has_field(PythonResolveField):
resolve = tgt[PythonResolveField].normalized_value(python_setup)
resolves_to_addresses[resolve].append(tgt.address.spec)
elif tgt.has_field(PythonRequirementCompatibleResolvesField):
resolves = tgt[PythonRequirementCompatibleResolvesField].normalized_value(
python_setup
)
for resolve in resolves:
resolves_to_addresses[resolve].append(tgt.address.spec)
formatted_resolve_lists = "\n\n".join(
f"{resolve}:\n{bullet_list(sorted(addresses))}"
for resolve, addresses in sorted(resolves_to_addresses.items())
)
super().__init__(
f"{msg_prefix}:\n\n"
f"{formatted_resolve_lists}\n\n"
"Targets which will be used together must all have the same resolve (from the "
f"[resolve]({doc_url('reference-python_test#codeexperimental_resolvecode')}) or "
f"[compatible_resolves]({doc_url('reference-python_requirement#codeexperimental_compatible_resolvescode')}) "
"fields) in common."
)

And it will trigger with repl already becauase we use pex_from_targets.py. The only difference with this PR is that we will more eagerly throw that exception, specifically with a new msg_suffix that gives you the JQ workaround. Do we believe giving you the clunky JQ + peek workaround to run is better than saying no workaround at all? I think so. Wdyt?

@stuhood
Copy link
Member

stuhood commented Feb 1, 2022

Do we believe giving you the clunky JQ + peek workaround to run is better than saying no workaround at all? I think so. Wdyt?

I think that it is worthwhile, yea. But maybe not a high priority. I think that "filter by field value" is worth doing sooner rather than later in support of #13882, and we can recommend it more widely / link to a page about it.

@Eric-Arellano
Copy link
Contributor Author

But maybe not a high priority.

It's easy to finish - all that's left is figuring out the JQ snippet, then writing one simple unit test. I'm not very fluent with JQ but I think we have enough examples in the docs I could scrape something together.

@stuhood
Copy link
Member

stuhood commented Feb 1, 2022

I'm not very fluent with JQ but I think we have enough examples in the docs I could scrape something together.

I added an example here: https://docs.google.com/document/d/1mzZWnXiE6OkgMH_Dm7WeA3ck9veusQmr_c6GWPb_tsQ/edit#heading=h.hzxpmsxjb6jw

@Eric-Arellano Eric-Arellano restored the repl-multiple-resolves branch February 2, 2022 19:57
@Eric-Arellano Eric-Arellano reopened this Feb 2, 2022
…e resolves

[ci skip-rust]

[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@Eric-Arellano Eric-Arellano marked this pull request as ready for review February 2, 2022 20:47
@Eric-Arellano
Copy link
Contributor Author

Bump

@Eric-Arellano Eric-Arellano merged commit c2f6404 into pantsbuild:main Feb 5, 2022
@Eric-Arellano Eric-Arellano deleted the repl-multiple-resolves branch February 5, 2022 04:49
Eric-Arellano added a commit that referenced this pull request Feb 11, 2022
We generate a distinct virtualenv for each resolve, using the path `dist/python/virtualenvs/<resolve>`. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the `peek` snippet from #14327.

--

If `enable_resolves = false`, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to `[python].default_resolve`, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to `enable-resolves` that the old path will no longer be valid.

[ci skip-rust]
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Feb 11, 2022
…14436)

We generate a distinct virtualenv for each resolve, using the path `dist/python/virtualenvs/<resolve>`. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the `peek` snippet from pantsbuild#14327.

--

If `enable_resolves = false`, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to `[python].default_resolve`, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to `enable-resolves` that the old path will no longer be valid.

[ci skip-rust]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Eric-Arellano added a commit that referenced this pull request Feb 11, 2022
… of #14436) (#14454)

We generate a distinct virtualenv for each resolve, using the path `dist/python/virtualenvs/<resolve>`. From there, the user can load in their IDE which one to use.

If users want to only generate for one IDE, they can use the `peek` snippet from #14327.

--

If `enable_resolves = false`, we stick with the old behavior. This is convenient because it would be a misnomer to write the path to `[python].default_resolve`, which doesn't make sense to use if resolves aren't enabled.

We log a warning when you do migrate to `enable-resolves` that the old path will no longer be valid.

[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