-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(): Add logs for pip environment when installing #8266
Conversation
Hmm... This is too late IMO. We should log this immediately at the start of the install command, instead of just-before-installation. And, it's probably better off as a |
src/pip/_internal/req/__init__.py
Outdated
@@ -46,7 +47,7 @@ def install_given_reqs( | |||
|
|||
(to be called after having downloaded and unpacked the packages) | |||
""" | |||
|
|||
logger.info(get_pip_version()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in another comment by @pradyunsg , a better place for this would be somewhere in commands.install:run to show the message at the start , but you might have to experiment a bit about where to put the logger.debug
message within that function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also saw that there was a request about adding the install location of the packages too, which I think is currently missing.
In that affect, having a custom function which gets this information along with pip version, and then writing a custom logger message might be better than directly relying on get_pip_version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason on putting the logger call here was mainly due to this comment #3166 (comment)
Yeah, though I guess, I would look for other possible places as well.
news/3166.feature
Outdated
@@ -0,0 +1 @@ | |||
Logs information about pip environemnt when installing a package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo environemnt
-> environment
Also, I am a little confused if the news entry should go as |
bf1b995
to
466eb27
Compare
src/pip/_internal/req/__init__.py
Outdated
@@ -46,7 +46,6 @@ def install_given_reqs( | |||
|
|||
(to be called after having downloaded and unpacked the packages) | |||
""" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like that a newline was removed from this file by accident. You can re-add it so that this change doesn't show up as part of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh! yeah, I'll update it in subsequent commits.
I am finding it difficult to get the install location beforehand. There are a number of option that user can provide to change the install location like I found |
There's |
It's probably better to figure out what to do for the install location logging in a follow up PR. |
466eb27
to
4e87ecd
Compare
@@ -249,6 +250,7 @@ def run(self, options, args): | |||
|
|||
install_options = options.install_options or [] | |||
|
|||
logger.debug("Using {}".format(get_pip_version())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a test can be added to verify that the logger contains this string when install in called with -v
, something along the lines of tests.lib.test_lib:https://github.com/pypa/pip/blob/master/tests/lib/test_lib.py#L40, maybe a smaller version of it.
4e87ecd
to
eeff6e8
Compare
@McSinyx, AFAICU, the
However, I still don't get how we can find the concrete location! 😐 |
@gutsytechster, IIUC these are concrete locations ( |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
eeff6e8
to
a0d37a1
Compare
tests/functional/test_install.py
Outdated
|
||
|
||
def test_install_logs_pip_version_in_debug(script): | ||
result = script.pip('install', '-v', 'INITools==0.2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test uses the network. I'd suggest to install a package from the fixture shared_data
.
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
e14fd31
to
277b1e6
Compare
Thanks @gutsytechster! |
Towards #3166
Now, when the installation is performed the logs are shown as
Please focus on last third line. Also, I'd like to get reviews about, if it should go as
logger.info
or aslogger.debug
.