-
-
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
Expose rules from language backends with an application to python_dist() creation #5747
Expose rules from language backends with an application to python_dist() creation #5747
Conversation
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.
This looks great... thanks!
@@ -71,6 +71,8 @@ def register_bootstrap_options(cls, register): | |||
default=['pants.backend.graph_info', | |||
'pants.backend.python', | |||
'pants.backend.jvm', | |||
'pants.backend.native', | |||
'pants.backend.python', |
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.
python
is already in the list.
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.
Fixed in 241692f! Thanks.
@@ -103,6 +103,9 @@ def _copy_sources(self, dist_tgt, dist_target_dir): | |||
src_relative_to_target_base) | |||
shutil.copyfile(abs_src_path, src_rel_to_results_dir) | |||
|
|||
def _request_single(self, product, subject): | |||
return self.context._scheduler.product_request(product, [subject])[0] |
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.
This technically is not supposed to be exposed yet... should leave a comment here pointing to #5440.
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.
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.
This is really cool! I'd like to see some more tests of the loading functionality
@@ -169,11 +174,14 @@ def setup_legacy_graph(pants_ignore_patterns, | |||
|
|||
# Create a Scheduler containing graph and filesystem tasks, with no installed goals. The | |||
# LegacyBuildGraph will explicitly request the products it needs. | |||
if not rules: | |||
rules = [] |
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.
This param defaulting feels like it should live above the comment about creating the scheduler. Maybe after if not build_file_aliases
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.
Moved to after if not build_file_aliases
in 4cb2a18!
""" | ||
|
||
if not isinstance(rules, Iterable): | ||
raise TypeError('The rules must be an iterable, given {}'.format(rules)) |
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.
Might be good to explicitly use the repr by changing {}
to {!r}
.
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.
Done in 4cb2a18!
from pants.util.osutil import get_normalized_os_name | ||
|
||
|
||
class Platform(datatype('Platform', ['normed_os_name'])): |
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.
maybe use normalized_os_name
?
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.
Done in 056c125!
@@ -144,4 +144,8 @@ def invoke_entrypoint(name): | |||
if subsystems: | |||
build_configuration.register_subsystems(subsystems) | |||
|
|||
rules = invoke_entrypoint('rules') | |||
if rules: | |||
build_configuration.register_rules(rules) |
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.
I'd like to see some unit tests for this in test_extension_loader.py
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.
It's only 3 lines long... seems like overkill?
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.
Fair. My thought was that as this is a plugin-writer facing change, it should have tests. If we're currently thinking of it as being experimental, then it's fine without. It'd be nice if that were noted somehow though.
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.
It's definitely covered by tests, in that the rest of this change relies on it working... it just doesn't seem worth a unit test. It's definitely not code with high cyclomatic complexity, for example.
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.
I totally agree that it's important to test anything that goes out to plugin writers, but as far as I am aware this change only affects backends within the pants codebase. I did however write an extremely small test anyway (see here). The natural extension of this PR is to do the same for plugins, and I absolutely think that change be paired with testprojects using it and integration tests as well, probably, but for this PR I think passing the python dist testing that exists is sufficient to prove it works.
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!
CI is failing at compiling native code and the error message isn't visible because pex decodes the stdout/stderr to ascii in their |
Ok, so setting aside the above, CI is also failing when building the engine for either platform with (on osx):
This rule is not exhibiting this issue locally (on osx) in the python dist integration test, for example (which completes successfully), but when I run that command above locally (on osx) I get the same error (not |
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 for the changes. Looks good, modulo passing CI.
from pants.util.osutil import get_normalized_os_name | ||
|
||
|
||
class Platform(datatype('Platform', ['normalized_os_name'])): |
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.
I think this should have a docstring, if you've got a good idea for one handy.
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.
I do -- this datatype was ripped from my real dev branch here which has a ton more changes (and Platform isn't defined in register.py, for example), so I was a little hasty. I want to leave the docstring off for now because I'm going to move this class out of register.py for sure immediately after this pr gets merged (and keeping the docstring out makes it less legitimate).
Does that make sense? I can totally add the docstring too with great ease, just wanted to get your thoughts on my thoughts.
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.
That works for me.
1418e08
to
94f68f5
Compare
Problem
It's not possible to use rules defined outside of
src/python/pants/engine
in the rest of Pants.Solution
Modify
load_backend()
inextension_loader.py
to understand arules
entrypoint. If a backend has a method namedrules()
defined in its register.py, that method will be invoked to get a list of rules to make the scheduler with.Result
We can now use rules defined in language backends. I created a
register.py
in the native backend so that we can test that this works (it's consumed in BuildLocalPythonDistributions). I have a diff that expands the native backend which depends on this functionality.Note: Extending this to cover plugins as well is trivial (make the exact same change, but to
load_plugins()
instead).