Skip to content

Don't use editable mode in tox#21

Merged
babolivier merged 2 commits intomainfrom
baboliver/develop
Dec 14, 2021
Merged

Don't use editable mode in tox#21
babolivier merged 2 commits intomainfrom
baboliver/develop

Conversation

@babolivier
Copy link
Copy Markdown
Contributor

Fixes #20

@babolivier babolivier requested a review from a team as a code owner December 2, 2021 11:29
@callahad
Copy link
Copy Markdown

callahad commented Dec 2, 2021

Related conversation in #18

@callahad
Copy link
Copy Markdown

callahad commented Dec 2, 2021

I'm curious if not using editable installs at all is possible? Sounds like avoiding editable installs prevents whole classes of potential packaging bugs.

@babolivier
Copy link
Copy Markdown
Contributor Author

babolivier commented Dec 2, 2021

I'm curious if not using editable installs at all is possible? Sounds like avoiding editable installs prevents whole classes of potential packaging bugs.

The comment says it's needed for trial, so I just assumed that was the reason, but I managed to have it run just fine with only this config for the py environment:

[testenv:py]

extras = dev

commands =
  python -m twisted.trial tests

@reivilibre do you remember what lead to this conclusion?

@callahad
Copy link
Copy Markdown

callahad commented Dec 2, 2021

Looks like it's from this commit four years ago matrix-org/synapse@4dd61df. Suspect things may have changed since then.

@reivilibre
Copy link
Copy Markdown
Collaborator

Yeah, I copied it from Synapse because:

  • consistency
  • it sounded logical and reasonable
  • I've had issues in the past with Trial and where it looks for tests to run.

The reason it seems to work here is because of the -m flag to Python, which afaik adds modules in the same directory to the module path as a side-effect of what it does. If you replace python -m twisted.trial with just trial you'll find it doesn't work.

That doesn't concern me however, since it's Tox and we can just use python -m twisted.trial. So I'm happy for you to remove editable mode if you find that helps things.

You will notice that using non-editable mode will be slower because it has to reinstall the package (but not necessarily the deps) every time, whereas in editable mode this is a no-op. However, editable mode is currently broken so the point is moot...

(unless that meant we preferred a import setuptools; setuptools.setup() setup.py file for compatibility — I'm not super fussed about which way you choose to do it as I don't habitually run through tox anyway... :P)

@babolivier
Copy link
Copy Markdown
Contributor Author

Right, let's drop the editable mode then. We're talking about modules here anyway, which are usually fairly small, so the time save introduced by using the editable mode (even if usedevelop worked) probably doesn't outweigh the additional reliability in packaging not using it brings.

@babolivier babolivier changed the title Use deps = --editable .[dev] instead of usedevelop=true Don't use editable mode in tox Dec 2, 2021
@richvdh
Copy link
Copy Markdown
Member

richvdh commented Dec 2, 2021

The reason it seems to work here is because of the -m flag to Python, which afaik adds modules in the same directory to the module path as a side-effect of what it does.

the same directory as... what?

@reivilibre
Copy link
Copy Markdown
Collaborator

reivilibre commented Dec 2, 2021

the same directory as... what?

the present working directory.

rei@lithium ┄┄ /tmp $ python -m ptpython
>>> import sys
>>> sys.path
['', '/tmp', '/usr/lib/python39.zip', '/usr/lib/python3.9', '/usr/lib/python3.9/lib-dynload', '/home/rei/.local/lib/python3.9/site-packages', '/home/rei/work/PS/synapse-my-module', '/usr/local/lib/python3.9/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.9/dist-packages']

>>>                                                                                                                                                                                                                                                                                                                          

rei@lithium ┄┄ /tmp $ ptpython
>>> import sys
>>> sys.path
['', '/home/rei/.local/bin', '/usr/lib/python39.zip', '/usr/lib/python3.9', '/usr/lib/python3.9/lib-dynload', '/home/rei/.local/lib/python3.9/site-packages', '/home/rei/work/PS/synapse-my-module', '/usr/local/lib/python3.9/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.9/dist-packages']

>>>

Can't promise I'm not blaming the wrong thing — but all tools that I use that interact with the path seem to be affected this way (which has really confused me in the past until I noticed).

edit: I think the first '' entry might be added by ptpython, so bad example (it is a REPL after all — didn't have time to craft something more specialist)

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Dec 2, 2021

the present working directory.

how does that help it find the tests? Doesn't tox chdir to the right .tox directory?

@reivilibre
Copy link
Copy Markdown
Collaborator

the present working directory.

how does that help it find the tests? Doesn't tox chdir to the right .tox directory?

No... (or at least not in this case? I imagine you must have some reason for thinking it might!)

Reproduction:

rei@lithium ┄┄ ~/work/PS/synapse-my-module $ cat tests/__init__.py 
import os
print("cwd ", os.getcwd())

then

rei@lithium ┄┄ ~/work/PS/synapse-my-module $ tox -e py
(snip)

py run-test: commands[0] | python -m twisted.trial tests
cwd  /home/rei/work/PS/synapse-my-module 

(snip)

Copy link
Copy Markdown
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections here!

I found the discussion hard to follow. The summary appears to be:
#18 happens when we use usedevelop=True, since that option expects a setup.py. Removing it makes tox work twice in a row, as noted in the issue.
We originally added usedevelop=True in Synapse as a workaround [link] which does not appear to be necessary for how we invoke trial [link].

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Running the py tox environment twice in a row fails

5 participants