Skip to content
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

tox 3.0.0rc1 breaks devpi test #755

Closed
fschulze opened this issue Jan 29, 2018 · 6 comments
Closed

tox 3.0.0rc1 breaks devpi test #755

fschulze opened this issue Jan 29, 2018 · 6 comments

Comments

@fschulze
Copy link
Contributor

@fschulze fschulze commented Jan 29, 2018

From #728 (comment)

The change of cmdline in f85b827 broke devpi test.

It's easy to fix on the devpi side, but I'm not sure if it will also break detox. I wasn't able to get detox tests running when I tried.

Maybe it would be better to revert cmdline back to test.session.main and change the entry point to tox.session.run_main instead.

cc @gaborbernat @obestwalter

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 29, 2018

Thanks @fschulze can you explain what broke or post a link to a CI run, where that happened? Is devpi relying on some tox internals there? As we don't really have a programmatic API for tox (at least thougt so), maybe we need to introduce one to prevent breakhages like this in the future? @tonybaloney also uses programmatic access to tox, so this might be something worth thinking about.

@fschulze
Copy link
Contributor Author

@fschulze fschulze commented Jan 29, 2018

It's used here:
https://github.com/devpi/devpi/blob/master/client/devpi/test.py#L119
and here:
https://github.com/devpi/devpi/blob/master/client/devpi/test.py#L101

I haven't touched that code myself. I'm pretty sure the intention is to basically vendor tox and not run it as a subprocess which involves looking up the correct binary etc.

The quick fix on our end is this:

diff --git a/client/devpi/test.py b/client/devpi/test.py
index 8f91f83c..09463940 100644
--- a/client/devpi/test.py
+++ b/client/devpi/test.py
@@ -9,7 +9,6 @@ import pkg_resources
 import py
 from devpi_common.archive import Archive
 import json
-import tox
 
 from devpi_common.url import URL
 from devpi_common.metadata import get_sorted_versions
@@ -116,7 +115,8 @@ class DevIndex:
             import detox.main
             return detox.main.main
         else:
-            return tox.cmdline
+            from tox.session import main
+            return main
 
     def get_tox_args(self, unpack_path):
         hub = self.hub

But I think it's better if it's fixed on the tox side to avoid forcing people to update both devpi-client and tox and getting support requests for older devpi-client versions when used with newer tox.

I haven't looked at how detox does things and if it's also affected by this.

@gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Jan 29, 2018

eh, @obestwalter said the only official API is the command line invocation, calling into tox code via import is sort of uncharted territory 🤔

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 30, 2018

Hi @gaborbernat this is he difference between the ideal and the real world - the 3.0 sounds less random now I guess ...

I would like to collect problems like these for a bit longer and see how we should proceed with this release. Maybe grepping through github what people do programmatically with tox might be a good idea. I don't think it is many projects that would be affected by these changes.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jan 30, 2018

I took a quick look, and tox.cmdline definitely shouldn't be removed, IMHO:

Seeing that it's been in the documentation for a long time (and still is), it definitely seems like an official API to me...

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 30, 2018

Hi @The-Compiler, thanks for checking that, those docs would have had to be changed or removed as part of that change (I am not even sure if they are still working as documented in current releases, because I don't use them).

@gaborbernat that we don't have an offficial programmatic API is not something coming from me but from @hpk42 himself (as expressed in the CHANGELOG entry of the 2.0 release):

rename internal files -- tox offers no external API except for the experimental plugin hooks, use tox internals at your own risk.

But if this is even in the docs, we don't have to be surprised that people are using it :) FWIW I don't see anything wrong with having a stable, importable tox.cmdline that gives acces to tox just the same way as if it would have been called from the command line directly. That basically is like a least common denominator API and will even be quite stable if we change a lot under the hood. And if command line parameters or behaviours of tox change then the behaviour of tox.cmdline changes accordingly, and noone using this should be surprised by that. I think though i should make myself familiar with the changes and the ideas behind it first, before I say the next stupid thing - so I'll look at the PR first :)

fschulze added a commit to fschulze/tox that referenced this issue Jan 30, 2018
…on and changing the entry point instead.
fschulze added a commit to fschulze/tox that referenced this issue Jan 30, 2018
…on and changing the entry point instead.
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants