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

Requirements on language-specific sources should be optional. #6375

Merged
merged 3 commits into from Aug 24, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Aug 21, 2018

E.g., a repo may have java without scala or vice versa.

This only works today because we implicitly expect codegen tasks
(which produce java and scala), and the deferred sources mapper task, to
be registered. But not having them registered is a totally
reasonable thing to do (e.g., it improves start up time because
there are fewer options to parse).

This extends the logic of #6357.

The goal is to be able to run pants with just the backends you want.
In fact, once this is possible we should remove all the default registrations
(keeping, say, just graph_info and project_info) and require you to
opt in to whatever lang-specific backends you need.

Debatably, many more, possibly even all, requirements should be optional.
But that's a bigger issue, for another day.

Requirements on language-specific sources should be optional.
E.g., a repo may have java without scala or vice versa.

This only works today because we implicitly expect codegen tasks
(which produce java and scala), and the deferred sources mapper task, to
be registered.  But not having them registered is a totally
reasonable thing to do (e.g., it improves start up time because
there are fewer options to parse).

This extends the logic of #6357.

The goal is to be able to run pants with just the backends you want.
In fact, once this is possible we should remove all the default registrations
(keeping, say, just graph_info and project_info) and require you to
opt in to whatever lang-specific backends you need.

Debatably, many more, possibly even all, requirements should be optional.
But that's a bigger issue, for another day.

@benjyw benjyw requested review from stuhood and jsirois Aug 21, 2018

@jsirois
Copy link
Member

jsirois left a comment

What happens when optional data is not there? Do tasks with these requirements need to be None bulletproofed?

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Aug 21, 2018

Possibly, I'm experimenting with this manually now. But this change alone shouldn't break anything that isn't already broken - deregistering the relevant producer tasks today would cause a "Could not satisfy data dependencies" error. After this change it might cause a less-readable error of some other kind (such as dereferencing None), which is admittedly worse, but not materially so. And we'd fix those up as we go, should we discover them.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 21, 2018

The early error on a missing product when un-installing a backend leads to the right conclusion - you can't do that. The late None error would be more confusing and more buggy. Perhaps this is eye of the beholder, but I'd definitely prefer to just:

  1. stick to java/scala since those have implicit producers
  2. validate the consumers of the other product types and if None-safe, great, if not, add guards
@stuhood
Copy link
Member

stuhood left a comment

I don't fully understand the implications of this change, so it feels like it would be useful to have a concrete usecase that exercises it.

Would a "constrained backend" integration test make sense? Or is that overkill here?

The goal is to be able to run pants with just the backends you want.
In fact, once this is possible we should remove all the default registrations
(keeping, say, just graph_info and project_info) and require you to
opt in to whatever lang-specific backends you need.

While I don't want to discourage reasonable moves in this direction, it's important to note that rules that don't run will have "zero cost" with v2+pantsd. So it should not be the case in the longer term that it is necessary to finetune your repo per-language like this.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Aug 21, 2018

v2+pantsd is not imminent though (although I do want to help hurry it along...)

Still, for hygiene I think it would be better to be able to do this in the current engine. At least it will validate some currently implicit and very unintuitive dependencies, such as "in your python-only repo you must have at least one task that produces the scala product"...

Anyway, this should be relatively easy to test. I will set something up and see how it goes.

Add tests to prove independence of python & jvm backends.
Also made the python dep on native optional. The test above
proves that this works.  The vast majority of python repos
do not need this dep, so we shouldn't require it.
@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Aug 22, 2018

I just pushed a couple of integration tests that prove that the python and jvm backends run with no other backends present (well, except for an internal backend in the JVM case, just because the example code's BUILD file requires it, with a TODO to fix that).

@jsirois
Copy link
Member

jsirois left a comment

Thanks for adding the tests!

@benjyw benjyw merged commit 42cf416 into pantsbuild:master Aug 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:optional_language_requirements branch Aug 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment