-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Support usage of scala_artifact
addresses in scalac_plugin
targets
#19205
Support usage of scala_artifact
addresses in scalac_plugin
targets
#19205
Conversation
The solution feels a bit work-aroundish and could probably be implemented in the graph by adding a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Non-blocking comment on the use of _TargetParametrizations
.
@rule | ||
async def _resolve_scalac_plugin_artifact( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code is invoked from a @rule
which will already memoize "the plugins to use for a target", I think that it should probably be a rule helper (plain async def
) instead of @rule
. That would avoid defining the request type, and the return type wrapping.
parametrizations = await Get( | ||
_TargetParametrizations, | ||
{ | ||
_TargetParametrizationsRequest( | ||
address.maybe_convert_to_target_generator(), | ||
description_of_origin=( | ||
f"the target generator {address.maybe_convert_to_target_generator()}" | ||
), | ||
): _TargetParametrizationsRequest, | ||
environment_name: EnvironmentName, | ||
}, | ||
) | ||
|
||
target = parametrizations.get_subset( | ||
address, request.consumer_target, field_defaults, target_types_to_generate_requests | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, as you've mentioned, _TargetParametrizations
is a very awkward API, which is part of why it is still explicitly private. I'd like to find a cleaner way to expose the get_subset
call... it may be that it is a good use-case for one of our first public rule helpers (rather than exposing the _TargetParametrizations
type, which should maybe be an implementation detail).
get_subset
is the fundamental operation, but it's possible that there is a better granularity to expose, such as one level up in resolve_dependencies
: it's also possible that exposing resolve_dependencies
as an API that can be used to expand any given list of dependencies would make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I did come up with this implementation by looking at the implementation of the resolve_dependencies
rule in the graph. However getting the subset is only one step in the process, that results in getting the actual target generator, we still need the final generated target and it's why there is a call to parametrizations.generated_for
right after.
Although exposing the subset will get a long way, because IIRC there is a dependency relationship between a target generator and its generated targets, so I guess I could resolve the dependencies of the generator and, in case I find none or more than one, then error there.
In a way, the field artifact
in scalac_plugin
is like a SingleDependencyField
for which we expect to find only one target after expanding parameters and target generation. What makes it even more awkward is the fact that the consumer target, the one that carries the parameters over, is not the scalac_plugin
itself, but the scala_sources
target that lists the plugin names in the scalar_plugins
field (a field that gives me SpecialCasedDependencies
vibes, but that's a different topic).
So, in a way, everything here smells of this being a special case:
- A target (
scalac_plugin
) that adds metadata to the build and it's meant to decorate another target (jvm_artifact
/scala_artifact
). And one that, in a way, feels like having a 1->1 dependency relationship with the decorated target. - A 1:1 target generator (
scala_artifact
->jvm_artifact
) - Parametrized fields
- A compile-only dependency between a
scala_source(s)
target and thejvm_artifact
target generated, and the dependency happens via an intermediary target (thescalac_plugin
) - Also, Scalac plugins can be globally defined in the
scalac
subsystem using adict
where the key is the resolve name.
In general, this is hard to generalise as only the Kotlin backend implements compiler plugins in a similar fashion and it's unlikely this scenario shows up in there too.
Sorry for the write up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, there is another case that I'm aware of where having a field for a single dependency would be nice: The helm_deployment
target. That target needs a reference to the Helm chart to deploy and there should be only one of those.
At first I wanted to model using a bespoke field but ended up asking users to add the address to the chart in the dependencies
field as I wanted Pants to rebuild any code that may have changed. While this works well, it feels unpolished and the implementation is a bit awkward as it has to avoid a rule cycle so I always wanted to change that and in fact I may have a branch somewhere with some changes.
Co-authored-by: Stu Hood <stuhood@gmail.com>
…r target, not the consumer
This updates `changelog.py` to write the (non-internal) changes to the relevant release notes file directly, rather than requiring a human to copy-paste it in. For now, the file is just mutated, without committing. The internal changes are still printed for the human to deal with. For instance, `pants run src/python/pants_release/changelog.py -- --prior 2.18.0.dev1 --new 2.18.0.dev2` adds a new section to `src/python/pants/notes/2.18.x.md`: ```diff diff --git a/src/python/pants/notes/2.18.x.md b/src/python/pants/notes/2.18.x.md index e648a4525c..d6668a24b1 100644 --- a/src/python/pants/notes/2.18.x.md +++ b/src/python/pants/notes/2.18.x.md @@ -1,5 +1,35 @@ # 2.18.x Release Series +## 2.18.0.dev2 (Jun 14, 2023) + +### New Features + +* Include complete platforms for FaaS environments for more reliable building ([#19253](#19253)) + +* Add experimental support for Rustfmt ([#18842](#18842)) + +* Helm deployment chart field ([#19234](#19234)) + +### Plugin API Changes + +* Replace `include_special_cased_deps` flag with `should_traverse_deps_predicate` ([#19272](#19272)) + +### Bug Fixes + +* Raise an error if isort can't read a config file ([#19294](#19294)) + +* Improve handling of additional files in Helm unit tests ([#19263](#19263)) + +* Add taplo to the release ([#19258](#19258)) + +* Handle `from foo import *` wildcard imports in Rust dep inference parser ([#19249](#19249)) + +* Support usage of `scala_artifact` addresses in `scalac_plugin` targets ([#19205](#19205)) + +### Performance + +* `scandir` returns `Stat`s relative to its directory. ([#19246](#19246)) + ## 2.18.0.dev1 (Jun 02, 2023) ### New Features ``` This also moves it into the new `pants_release` root, adds some basic tests, and has it fetch the relevant branch directly, rather than prompting the user to do so. It also pulls out a `git.py` support module with helpers for exec-ing `git` as an external process. The commits are individually reviewable. This is a step towards automating more of the "start release" process, per #19279. After this core refactoring of the functionality, I think the next step is to combine it with the CONTRIBUTORS update and version bumping, and then after that string it all together on CI so that starting a release is fully automated.
This allows to reference a
scala_artifact
target in theartifact
field of ascalac_plugin
target and enables the usage of scalac plugins in workspaces that cross-compile with different versions of Scala.For example like:
Before this, artifact resolution will fail since
scala_artifact
is a target generator and it is parametrized, therefore Pants can't find the given artifact just by looking up the address to the generator.