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

Fix `PytestRun` to handle multiple source roots. #5400

Merged
merged 1 commit into from Jan 28, 2018

Conversation

Projects
None yet
4 participants
@jsirois
Copy link
Member

jsirois commented Jan 27, 2018

Fix PytestRun to handle multiple source roots.

Although an attempt was made to handle multiple source roots when
mapping python source paths for py.test and coverage, there were
ambiguities in the coverage mapping in particular leading to the
inability to run tests and collect coverage for code under test across
all python source roots. The python task pipeline is amended to produce
a source pex per source root, allowing PytestRun in turn to be source
root aware in its execution of coverage reports and a multi-source-root
test is added to ensure this case is handled.

In addition, resource targets are duplicated to all source pexes
containing python code that needs access to the resources. Previously,
unrelated resources could also be added to the source pexes (e.g: a
Java resource) and this is fixed as well.

Fixes #5314
Fixes #5401

@jsirois jsirois force-pushed the jsirois:issues/5314 branch 4 times, most recently from 2df9b75 to 21b6c7b Jan 27, 2018

@jsirois jsirois requested review from stuhood , benjyw and cosmicexplorer Jan 28, 2018

@benjyw

benjyw approved these changes Jan 28, 2018

Copy link
Contributor

benjyw left a comment

Thanks for doing this heavy lifting! And for the excellent inline documentation.

context = self.context
python_target_addresses = [p.address for p in context.targets(predicate=is_python_target)]

targets_by_base = OrderedDict()

This comment has been minimized.

@benjyw

benjyw Jan 28, 2018

Contributor

Why does this need to be ordered? Could you use a defaultdict(set) instead and save yourself a little boilerplate below?

This comment has been minimized.

@jsirois

jsirois Jan 28, 2018

Member

Added a comment. The ordering is needed to preserve an ordering of ~PYTHONPATH over multiple source pexes.

for target_base, targets in targets_by_base.items():
for dependee in dependees:
if dependee in targets:
# N.B.: This can add the resource to too many pexes. A canonical example is

This comment has been minimized.

@benjyw

benjyw Jan 28, 2018

Contributor

In a comment above you mention that resource targets belong with their direct dependees. Here it seems like they will go in the same source pex as indirect ones too? So that comment should be clarified.

This comment has been minimized.

@jsirois

jsirois Jan 28, 2018

Member

Yeah, the two comments together and even apart were confusing. Clarified.

# the resource is added to both the test pex and the lib pex and it's only needed in the
# lib pex. The upshot is we allow python code access to undeclared (direct) resource
# dependencies which is no worse than historical precedent, but could be improved with a
# more complex algorithm.

This comment has been minimized.

@benjyw

benjyw Jan 28, 2018

Contributor

Why is it complex? E.g., can we not easily get all the direct-only dependees of a target?

This comment has been minimized.

@jsirois

jsirois Jan 28, 2018

Member

I think direct-only is not what we want. Say we had python_library -> target -> resources - ie an indirect dep via an aggregator target.

@@ -23,11 +23,15 @@
from pants.python.python_repos import PythonRepos


def has_python_sources(tgt):
def is_python_target(tgt):
# We'd like to take all PythonTarget subclasses, but currently PythonThriftLibrary and
# PythonAntlrLibrary extend PythonTarget, and until we fix that (which we can't do until

This comment has been minimized.

@benjyw

benjyw Jan 28, 2018

Contributor

Not for this change, but this reminds me that we can now fix this...

@jsirois jsirois force-pushed the jsirois:issues/5314 branch from 3dfd034 to 414f988 Jan 28, 2018

@benjyw

benjyw approved these changes Jan 28, 2018

Fix `PytestRun` to handle multiple source roots.
Although an attempt was made to handle multiple source roots when
mapping python source paths for `py.test` and `coverage`, there were
ambiguities in the `coverage` mapping in particular leading to the
inability to run tests and collect coverage for code under test across
all python source roots. The python task pipeline is amended to produce
a source pex per source root, allowing `PytestRun` in turn to be source
root aware in its execution of coverage reports and a multi-source-root
test is added to ensure this case is handled.

In addition, resource targets are duplicated to all source pexes
containing python code that needs access to the resources. Previously,
unrelated resources could also be added to the source pexes (e.g: a
Java resource) and this is fixed as well.

Fixes #5314
Fixes #5401

@jsirois jsirois force-pushed the jsirois:issues/5314 branch from 414f988 to 43166a5 Jan 28, 2018

@jsirois jsirois merged commit 1a4dcfc into pantsbuild:master Jan 28, 2018

1 check passed

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

@jsirois jsirois deleted the jsirois:issues/5314 branch Jan 28, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 29, 2018

Noting that this cannot be trivially cherry-picked to 1.4.x due to the rename / delete of the old python task pipeline. If the change is deemed desirable in 1.4.x some fairly heavy manual merging will need to be done. I'll hold off doing this until / if the need is established.

@stuhood stuhood added this to the 1.4.x milestone Jan 29, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jan 29, 2018

...oh, I see.

That should be ok... that's happened a few times so far. I cherry-pick them by

  1. attempting the cherry-pick (to pick up the author and etc),
  2. reset+checkouting the conflicted renamed files,
  3. git-show ing the conflicted files from the relevant commit into a file and editing it to apply to the new locations,
  4. git-applying the edited diff,
  5. committing the cherry-pick, which will use the original message/author/etc
@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 29, 2018

K - I'll give this a whack.

jsirois added a commit to jsirois/pants that referenced this pull request Jan 30, 2018

Fix `PytestRun` to handle multiple source roots. (pantsbuild#5400)
Although an attempt was made to handle multiple source roots when
mapping python source paths for `py.test` and `coverage`, there were
ambiguities in the `coverage` mapping in particular leading to the
inability to run tests and collect coverage for code under test across
all python source roots. The python task pipeline is amended to produce
a source pex per source root, allowing `PytestRun` in turn to be source
root aware in its execution of coverage reports and a multi-source-root
test is added to ensure this case is handled.

In addition, resource targets are duplicated to all source pexes
containing python code that needs access to the resources. Previously,
unrelated resources could also be added to the source pexes (e.g: a
Java resource) and this is fixed as well.

Fixes pantsbuild#5314
Fixes pantsbuild#5401

jsirois added a commit that referenced this pull request Jan 30, 2018

Fix `PytestRun` to handle multiple source roots. (#5400) (#5407)
Although an attempt was made to handle multiple source roots when
mapping python source paths for `py.test` and `coverage`, there were
ambiguities in the `coverage` mapping in particular leading to the
inability to run tests and collect coverage for code under test across
all python source roots. The python task pipeline is amended to produce
a source pex per source root, allowing `PytestRun` in turn to be source
root aware in its execution of coverage reports and a multi-source-root
test is added to ensure this case is handled.

In addition, resource targets are duplicated to all source pexes
containing python code that needs access to the resources. Previously,
unrelated resources could also be added to the source pexes (e.g: a
Java resource) and this is fixed as well.

Fixes #5314
Fixes #5401
@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jan 30, 2018

Cherry-picked via #5407

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Feb 1, 2018

Thanks John!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Feb 2, 2018

It looks like this one causes a significant number of errors in our internal sandboxes for an as-of-yet unknown reason. Will investigate tomorrow.

# the resource is added to both the test pex and the lib pex and it's only needed in the
# lib pex. The upshot is we allow python code access to undeclared (ie: indirect)
# resource dependencies which is no worse than historical precedent, but could be
# improved with a more complex algorithm.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 2, 2018

Contributor

Will this causing resource confliction? i.e. in a case of test -> lib + resource; lib -> resource, where test contains the same resources and using it for test purpose.

This comment has been minimized.

@jsirois

jsirois Feb 2, 2018

Member

I used OrderedDicts in this change with the express purpose of preserving ~PYTHONPATH ordering, so I think this works out correctly but it does deserve a unit test. I'll add that in a follow-up PR and I've clarified the case you're describing - I think - in #5425 @UnrememberMe if you don't mind checking that issue description to make sure I've covered your concern, I'd be grateful.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Feb 2, 2018

It looks like this one causes a significant number of errors in our internal sandboxes for an as-of-yet unknown reason. Will investigate tomorrow.

Stu gave me enough info offline to identify the issue, tracked in #5426

stuhood added a commit that referenced this pull request Feb 2, 2018

wisechengyi added a commit to wisechengyi/pants that referenced this pull request Feb 21, 2018

@wisechengyi wisechengyi referenced this pull request Feb 21, 2018

Closed

[wip] Revert 691d4d1 #5498

wisechengyi added a commit to wisechengyi/pants that referenced this pull request Feb 23, 2018

wisechengyi added a commit that referenced this pull request Feb 23, 2018

jsirois added a commit to jsirois/pants that referenced this pull request Mar 1, 2018

Generate a single python source chroot.
This change reverts the majority of pantsbuild#5400 (GatherSources) and adjusts
the new coverage plugin path mapping system to handle mapping a single
source chroot back to potentially multiple source roots. The existing
tests of this  capability introduced in pantsbuild#5400 still pass and as a result
the previously conflicting use cases represented by pantsbuild#5314 (multiple
library/binary source roots with interdependencies under test) and
pantsbuild#5426 (parallel test packages in a test source root) are now both
satisfied.

Fixes pantsbuild#5314
Fixes pantsbuild#5426
Depends on pantsbuild#5534

wisechengyi added a commit that referenced this pull request Mar 1, 2018

Generate a single python source chroot.
This change reverts the majority of #5400 (GatherSources) and adjusts
the new coverage plugin path mapping system to handle mapping a single
source chroot back to potentially multiple source roots. The existing
tests of this  capability introduced in #5400 still pass and as a result
the previously conflicting use cases represented by #5314 (multiple
library/binary source roots with interdependencies under test) and
#5426 (parallel test packages in a test source root) are now both
satisfied.

Fixes #5314
Fixes #5426
Depends on #5534

jsirois added a commit to jsirois/pants that referenced this pull request Mar 1, 2018

Generate a single python source chroot.
This change reverts the majority of pantsbuild#5400 (GatherSources) and adjusts
the new coverage plugin path mapping system to handle mapping a single
source chroot back to potentially multiple source roots. The existing
tests of this  capability introduced in pantsbuild#5400 still pass and as a result
the previously conflicting use cases represented by pantsbuild#5314 (multiple
library/binary source roots with interdependencies under test) and
pantsbuild#5426 (parallel test packages in a test source root) are now both
satisfied.

Fixes pantsbuild#5314
Fixes pantsbuild#5426
Depends on pantsbuild#5534

jsirois added a commit that referenced this pull request Mar 2, 2018

Generate a single python source chroot. (#5535)
This change reverts the majority of #5400 (GatherSources) and adjusts
the new coverage plugin path mapping system to handle mapping a single
source chroot back to potentially multiple source roots. The existing
tests of this  capability introduced in #5400 still pass and as a result
the previously conflicting use cases represented by #5314 (multiple
library/binary source roots with interdependencies under test) and
#5426 (parallel test packages in a test source root) are now both
satisfied.

Fixes #5314
Fixes #5426
Depends on #5534
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment