Let plugins declare @rules and move the native backend into contrib

[cosmicexplorer]

Problem

We want to move the native backend entirely into contrib in #5815, but we can't declare @rules in plugins (or contrib packages) yet. We added the ability to declare rules in backends in #5747 -- this is the corresponding change for plugins.

We want to move the native backend into contrib because for now the utility is limited to python_dist() creation, and because it brings in multiple large downloads (the native toolchain) that aren't necessary if the user doesn't want to compile any native code. This may also affect the number of downloads we see of these large packages, because we were pulling them in unconditionally before -- see #5780 for further context.

Solution

• Add rules from a rules() method in a plugin's register.py into the build configuration.
• Move the native backend and (still skipped) testing into contrib/native.
• Create pantsbuild.pants.contrib.native plugin and register it.
• Introduce NativeToolchainEnvironment in setup_py.py and depend on it as an optional product in BuildLocalPythonDistributions.
• Register @rules for generating NativeToolchainEnvironment in the contrib/native plugin. Register a shell PopulateNativeEnvironment task which simply requests a NativeToolchainEnvironment from the scheduler and places it into self.context.products (since there's not a fully formed story on extending @rules yet).

Result

Plugins can now declare @rules by defining a rules() method in their register.py, and all of the native toolchain has been moved to contrib/native, which provides an end-to-end of the new functionality, making it very simple to move the rest of the native backend into contrib/native in #5815 before merging.

[benjyw]

Why do we want to move the native backend to contrib?

[cosmicexplorer]

Moving the native backend to contrib was based off a discussion I had with @stuhood that happened haphazardly in #engine and #native yesterday -- there are two concerns here. The first is that we're unconditionally pulling down gcc and clang right now for building any python_dist(), regardless of whether that python_dist() has native sources -- that is an artifact of a (hasty) implementation, and is fixed by #5815, so it's not a reason to move anything to contrib by itself, but once it was mentioned I realized that it would be very easy to move the entirety of the native backend into contrib, and it provided an excuse to test adding @rules to plugin register.pys (where previously we could only declare them in backends). I think it would make a lot of sense to move python_dist() into contrib as well, since the way it stitches things into the build graph means there's not any new product dependencies to declare. It also seems to make sense to me (and this is what @stuhood was saying from what I recall) that because (at least right now) we can split this functionality off into contrib pretty cleanly, it would make sense to do so for the same reasons we have anything else in contrib (to separate orthogonal functionality, reduce the pex size for consumers that don't need it, faster to iterate on implementation because other parts of pants don't depend on it). But I'm not familiar with the expectations/guidelines/etc about what should or shouldn't be in contrib so if that sounds bogus it doesn't need to happen!

[jsirois]

It's probably unfortunate that today contrib == plugin. It seems a clear win to have native (and most everything) as a plugin, it's only the 'contrib' connotation that doesn't really fit here. It's true we do have the backends mechanism, but it's unweildy and ~unusable except by Pants PHDs as a Pants trimming customization mechanism.

[benjyw]

Yeah, I guess my only beef is with the name "contrib". In my mind "contrib" means "outside contribution of peripheral importance, possibly unstable". But this native backend was written, reviewed and tested by core Pants team members, so I think it should be on the same footing as Python and JVM. And I also think Go and possibly Node should be in that category, for similar reasons.

I agree with @jsirois that, in terms of the discovery and loading mechanism, everything should be plugins, even JVM and Python, and backends should probably go away entirely. But I think there's still value to the distinction between "this is core pants functionality with first-class support by the pants team" vs "here's possibly useful stuff that has been contributed, but you may need to ask the author for help with it" .

[benjyw]

Granted, this is all outside the scope of this change, and regardless if what we call it, it's true that you need to make native loadable as a plugin.

[benjyw]

Unrelated, seems to me that "Let plugins declare @rules" and "move the native backend into contrib" should be separate pull requests.

[jsirois]

"Let plugins declare @rules" and "move the native backend into contrib" should be separate pull requests.

+1

[stuhood]

Thanks: looks good.

[stuhood]

While I agree that this could be two PRs, the change to allow contrib modules to declare def rules is only a few lines, and is a slight extension of functionality added previously.

So my preference would be for just dropping it from the title.

[stuhood]

Let's wait to land this until after 1.8.x has been cut and stabilized a bit.

[cosmicexplorer]

Unrelated, seems to me that "Let plugins declare @rules" and "move the native backend into contrib" should be separate pull requests.

So your thoughts here I totally get -- my thought was that outside of making a whole new testproject (feasible!), there's no way I would be comfortable making the change to allow declaring @rules in plugins without having some form of end-to-end testing in a system that is exercised in CI (on top of what stu just mentioned). I agree that these really are two orthogonal concepts. Ended up thinking a bit more and wrote the below:

We discussed earlier today about merging #5815 after 1.8.x, not because of this PR alone but because a lot of related functionality is still in the pipeline, and nothing except excitement is pressuring us to get it in more quickly right now. Given that, I think it would be extremely reasonable to make:

1. A PR that adds a test suite for loading @rules from plugins, separate from any existing code. This allows us to iterate on this interface without having to tie it to orthogonal functionality. This should be a testproject and a single integration test at most.
2. A PR that moves the native backend into contrib/.

There are two things up for debate in (2): whether to move python_dist() functionality into contrib too, and whether to make a new directory named something like extensions to make the distinction that @benjyw was describing more useful. I 300% agree with @jsirois (and it's my impression that @stuhood does as well) that explicitly moving things out of core that can be moved is a goal -- there is literally no other way for us to dogfood the extension API (which is like, half of why I love pants), and it exposes assumptions made about shared global state (e.g. #5962), which is a great side effect.

@CMLivingston and @kwlzn -- can you speak to whether there would be a problem with moving python_dist() and the BuildLocalPythonDistributions task to contrib/(or a differently-named directory) along with the native backend? I ask because it seems specifically extremely easy (I can toss up a commit in this PR that does it if you want to see what that would look like), because BuildLocalPythonDistributions just inserts synthetic PythonRequirementLibrary targets, so there seem to be no hidden necessary dependencies e.g. on products. Some of the stuff in setup_py.py and definitely the python_dist() testing would be moved as well. I think it would allow for some coupling between python_dist() and the native backend as we iterate (since plugins can import one another), but it wouldn't enforce it. It seems like an extremely natural thing to do.

It seems like exposing @rules in backends can be tested similar, and probably should also be tested, since that allows us to work with something concrete if we want to iterate on the API for declaring @rules without immediately modifying the public register.py API for external plugins (which this PR does). Ping @baroquebobcat because you had some thoughts about this in #5747.

[cosmicexplorer]

Talked to @CMLivingston and now we're planning to move python_dist() into contrib as well (or a new dir) -- will look into that now.

[baroquebobcat]

Hm. I would like a test that specifically exercises the plugin rule loading, along the lines of the goal install test case https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/init/test_extension_loader.py#L270-L283

Otherwise it looks pretty good to me!

[CMLivingston]

It would be great if we could wait to forklift native into contrib/ until #5998 has been reviewed and merged. It is not a ton of extra work on either end but for the sake of a consistent and error free move into contrib, it would be great. We spoke about this earlier on Slack, sorry I did not comment this sooner.

Maybe we could separate that bit out into a different PR?

[cosmicexplorer]

Sounds like a plan!

…

On Thu, Jun 21, 2018 at 14:46 Chris Livingston ***@***.***> wrote: It would be great if we could wait to forklift native into contrib/ until #5998 <#5998> has been reviewed and merged. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5970 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPqT4_37t2e5d_Yfuf5DmVvL3sm7u8iks5t_BRFgaJpZM4UqXI1> .

[cosmicexplorer]

The rationale for landing @rules for plugins in the same PR as moving the native backend was because the native backend uses @rules liberally, and we would need the ability to declare them in order to move it wholesale. I think the appropriate way to do this now would be to:

1. Try to move some stub plugin in contrib/ or otherwise which declares a rule that the native or python backend consumes somehow (this was partially figured out in the current version of this PR), and add an integration test that just verifies it works (in a separate PR).
2. Move the rest of the native backend and python_dist() into contrib/ or otherwise, sharding across multiple PRs as necessary.

If #6652 gets resolved earlier (essentially by someone else doing (1) above), we can move directly to (2).

[stuhood]

I think the appropriate way to do this now would be to:

Sounds good. A variation on (1) might be to find a contrib plugin to add a @rule to.

[cosmicexplorer]

Plugins can already declare @rules, and the native backend is useful elsewhere in pants core, e.g. #6893, so both of the goal of this PR seem to have been invalidated.