fedora-python / tox-current-env

tox plugin to run tests in current Python environment
https://pypi.org/project/tox-current-env/
MIT License
23 stars 8 forks source link

hooks: override list_dependencies_command with dummy #32

Closed FFY00 closed 4 years ago

FFY00 commented 4 years ago

This gets rid of the pip dependency, undesirable in some cases.

Signed-off-by: Filipe Laíns lains@archlinux.org

FFY00 commented 4 years ago

I just tested and this indeed solves my issue.

FFY00 commented 4 years ago

Actually this does break the py38 installed: output, it will not be shown. It's not a big deal, just something to keep in mind.

hroncok commented 4 years ago

https://github.com/tox-dev/tox/blob/23dd96f5a6891fb13e298ea941bd457931421ffd/src/tox/config/__init__.py#L846 seems like it is a string, not a list, so the list here confuses me, is it indeed correct?

It's not a big deal, just something to keep in mind.

And document.

  1. Could you please remove "The installed: line in the output of tox --print-deps-only shows irrelevant output (based on the content of the real or faked virtual environment)." from the README?
  2. Could you please adapt the examples in the README? (How does the output look like now?)
  3. Could you please mention this new behavior somewhere in the README as well?
hroncok commented 4 years ago

I wonder if overriding https://tox.readthedocs.io/en/latest/plugins.html#tox.hookspecs.tox_runenvreport wouldn't be a better option.

FFY00 commented 4 years ago

It is correct, it's a "line-list" https://github.com/tox-dev/tox/blob/23dd96f5a6891fb13e298ea941bd457931421ffd/src/tox/config/__init__.py#L105.

We can override tox_runenvreport, yes. I imagined it could be overwritten by another plugin and that's why I suggested modifying list_dependencies_command, seems like a better place to do it. But looking at the documentation, it seems like tox_runenvreport is intended for this sort of cases, so we could use it instead.

Overriding tox_runenvreport actually works well for us because we can do it in code directly, as we are running tox in the same environment :grin:. What do you think? It would not break anything.

hroncok commented 4 years ago

I'd do it via tox_runenvreport, but please make sure to keep the normal behavior when the options are not used.

FFY00 commented 4 years ago

Okay, so I replaced tox's implementation with importlib.metadata. Unfortunately, this adds a importlib_metadata; python_version < '3.8' dependency. If you want, I can make tox_runenvreport soft-fail by returning unkown or something like that. Currently, we get the exact same output as before.

encukou commented 4 years ago

The dependency is fine IMO. But as Miro said, do make sure to keep the normal behavior when our options are not used. You can return None to skip the hook.

FFY00 commented 4 years ago

Ah, sorry! My bad :blush:

FFY00 commented 4 years ago

Friendly ping.

hroncok commented 4 years ago

Thanks for the updates.

This looks fine but I'd like to test it (manually) and add an automatic test as well. I will eventually get to it, but I cannot make any promises.

If you want to speed things up, please add a test yourself, although I realize that the tests might be a tad intimidating here. Worry not, I can answer questions!

hroncok commented 4 years ago

I've added the tests and modified the implementation a bit.

@FFY00 Could you please take a look at the changes?

FFY00 commented 4 years ago

Looks fine, maybe just add a test to make sure pip isn't used in tox_runenvreport.

hroncok commented 4 years ago

Looks fine, maybe just add a test to make sure pip isn't used in tox_runenvreport.

That has crossed my mind, but I had no idea how to do this effectively.

FFY00 commented 4 years ago

You can run in an isolated environment and monkeypatch PATH to be empty. The isolated environment should break sys.executable -m pip and an empty PATH should break pip and python -m pip.

FFY00 commented 4 years ago

Anyway, just an empty PATH should be enough to break tox's default implementation. You can go for that if it's enough for you.

hroncok commented 4 years ago

Empty PATH would prevent much more things, including running tox. I'd rather not block this PR on it