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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore pip freeze after pip install #2772

Closed

Conversation

adrien-berchet
Copy link

Fixes #2685

This is just an attempt to restore a very useful old behavior: printing the actual versions of the dependencies installed in the virtual environment. If this is the proper way to fix it, I will add tests, docs, etc.
WDYT @gaborbernat (or anyone else 馃槃 )?

Notes

  • The installed method uses the self._env.execute which make it difficult to format the output, so it displays a list like:
a==1.2.3
b==2.3.4
c==3.4.5

instead of the former one-line format:

a==1.2.3,b==2.3.4,c==3.4.5
  • For now I set show=True but maybe we want it to be customizable by the user.

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Please read in the issue my comments in which form I'd accept this, and do accordingly.

@adrien-berchet
Copy link
Author

Well, your comment is a bit short for someone like me that discovers the tox internals 馃槄
But I added a flag so now if we want to list the deps we have to call the following command: tox run -e py38 --display-dependencies. Is it what you have in mind?

@gaborbernat
Copy link
Member

Not really. I've said this should be on in CI envs and otherwise off.

@jugmac00
Copy link
Member

From just this week's experience.. debugging issues when the behavior is different locally in and in CI is a pain.

I am +1 for the attempt @adrien-berchet is suggesting.

@gaborbernat
Copy link
Member

We can keep the flag just the default value should change by CI env. I'm strong -1 on making this on by default locally.

@adrien-berchet
Copy link
Author

I am not sure I get your point @gaborbernat , in this current patch the default behavior is not changed, the user has to specify --display-dependencies to enable the new behavior (the show=True does not mean it will always be displayed, it just means that when the --display-dependencies is passed the deps are displayed regardless of the verbosity level). So in the CI we can pass this --display-dependencies argument and locally it's also possible to pass this argument if the user has any reason to do it, but by default it will not change anything.

@@ -143,6 +143,10 @@ def _setup_with_env(self) -> None:
else:
self._setup_pkg()

def _done_with_setup(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this, considering https://github.com/tox-dev/tox/blob/main/src/tox/tox_env/python/api.py#L216. Shouldn't we be reusing that call?

Copy link
Member

Choose a reason for hiding this comment

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

This is python specific so I don't see why it would leave under the generic runner and not the python scope.

Copy link
Member

Choose a reason for hiding this comment

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

So in the CI we can pass this --display-dependencies argument and locally it's also possible to pass this argument if the user has any reason to do it, but by default it will not change anything.

I'd argue in the CI envs we should do this without the user passing any flags. And that was my threshold of acceptability in the issue. Locally the user can still use the flag, if they want.

@gaborbernat
Copy link
Member

Superseded by #2794

@adrien-berchet
Copy link
Author

I just discovered #2784, it seems very nice!
I just think it could be useful to add some doc about this behavior and how to trigger it manually (e.g. just by defining a environment variable called CI).
Thank you very much @gaborbernat !

@adrien-berchet adrien-berchet deleted the restore_pip_freeze branch December 31, 2022 12:19
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.

Listing dependencies
3 participants