Opened 4 years ago

Closed 4 years ago

#6390 defect closed fixed (fixed)

Using trial -j can result in wrong paths in sys.path

Reported by: therve Owned by: therve
Priority: normal Milestone:
Component: trial Keywords:
Cc: Jonathan Lange, free Branch: branches/dist-trial-path-6390
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

The _preamble module in twisted/trial/_dist is fine for non-system install of Twisted, but it can mess up sys.path the way it calculates the Twisted path on system installs. We should remove that.

Change History (11)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 4 years ago by therve

Author: therve
Branch: branches/dist-trial-path-6390

(In [37809]) Branching to 'dist-trial-path-6390'

comment:3 Changed 4 years ago by therve

Keywords: review added
Owner: therve deleted

I tried something in the branch, by passing the direct value of sys.path as a separate environment variable to the workers. Using some tests it seemed that PYTHONPATH could result in weird order issues.

comment:4 Changed 4 years ago by Jean-Paul Calderone

Can you describe how this breaks things a bit more? Right now it's not clear to me what bug I should expect to be fixed in the branch. Although I do like the idea of removing this copy of _preamble.py for its own sake.

comment:5 Changed 4 years ago by therve

Sure. As a concrete example, using the twisted package on Ubuntu means that /usr/share/pyshared ends up in sys.path because that's where the .py files are, so it can confuse things up. trial itself is protected from that because we don't ship bin/_preamble, so the import fails and things go as normal, but we do ship the one in twisted/trial/_dist.

comment:6 Changed 4 years ago by free

Cc: free added

comment:7 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to therve

Is there any way to test this without needing to touch /usr? I'd like to set something up on buildbot to test this, but there aren't enough details here to figure out how to do that.

Presumably, there is other global state (I'm thinking of python -Wall is also not preserved.

comment:8 Changed 4 years ago by therve

Keywords: review added
Owner: therve deleted

There is certainly a way, maybe mangling sys.path before calling trial. I don't think it's worth having a buildbot for that though (even considering we don't even have a buildbot using trial -j).

Maybe you can open a bug about other python flags not being respected.

comment:10 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to therve

Thanks, therve.

  1. I think these pydoctor errors are spurious, but please make sure.
  2. I am unnerved by the fact that TRIAL_PYTHONPATH is now a wholly undocumented implementation detail; a trick made possible only by the fact that adding it doesn't involve adding any top-level suites. Please add some documentation to the module docstring of workertrial and the docstring of launchWorkerProcess explaining this side-effect. (Maybe a @see referencing each other so it's easy to find where the variable comes from / goes to.)
  3. The arguments variable in the test case is already really misleading (why not arguments.extend(args)?) and this change just makes it truly awful :). Please relay the value of env to the outer scope in a more readable way.
  4. There's no test coverage for the positive branch of workertrial, so nothing can verify the integration of the path setup. If you could find a way to put at least part of this into a function and test it that would be great.

These are all relatively minor points, so please address to your satisfaction and land.

comment:11 Changed 4 years ago by therve

Resolution: fixed
Status: newclosed

(In [38491]) Merge dist-trial-path-6390

Author: therve Reviewers: glyph, tom.prince Fixes: #6390

Remove the preamble setting the sys.path for trial dist children, and pass an environment variable instead.

Note: See TracTickets for help on using tickets.