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.

Sign up for GitHub

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

Jump to bottom

feat(): Add logs for pip environment when installing #8266

Merged
merged 3 commits into from Jul 10, 2020
Merged

feat(): Add logs for pip environment when installing #8266

merged 3 commits into from Jul 10, 2020

Conversation

gutsytechster
Copy link
Contributor

@gutsytechster gutsytechster commented May 19, 2020 •
edited

Towards #3166

Now, when the installation is performed the logs are shown as

(pip-env) gutsytechster@prashant:~/Documents/Projects/Open-Source-Projects/pip$ pip install django
Collecting django
  Using cached Django-3.0.6-py3-none-any.whl (7.5 MB)
Requirement already satisfied: pytz in /home/gutsytechster/miniconda3/envs/pip-env/lib/python3.6/site-packages (from django) (2020.1)
Requirement already satisfied: asgiref~=3.2 in /home/gutsytechster/miniconda3/envs/pip-env/lib/python3.6/site-packages (from django) (3.2.7)
Requirement already satisfied: sqlparse>=0.2.2 in /home/gutsytechster/miniconda3/envs/pip-env/lib/python3.6/site-packages (from django) (0.3.1)
pip 20.2.dev0 from /home/gutsytechster/miniconda3/envs/pip-env/lib/python3.6/site-packages/pip (python 3.6)
Installing collected packages: django
Successfully installed django-3.0.6

Please focus on last third line. Also, I'd like to get reviews about, if it should go as logger.info or aslogger.debug.

@pradyunsg
Copy link
Member

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 logger.debug call. :)

deveshks
deveshks reviewed May 19, 2020
View reviewed changes
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())
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

deveshks
deveshks reviewed May 19, 2020
View reviewed changes
news/3166.feature Outdated
@@ -0,0 +1 @@
Logs information about pip environemnt when installing a package.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo environemnt -> environment

@gutsytechster
Copy link
Contributor Author

Also, I am a little confused if the news entry should go as .feature or something else as this is not a bug, also not a documentation related issue.

deveshks
deveshks reviewed May 19, 2020
View reviewed changes
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)
"""

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@gutsytechster
Copy link
Contributor Author

gutsytechster commented May 19, 2020 •
edited

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 --root, --prefix, --target etc. Is there some utility that finds out the install location.

I found decide_user_install, but this mainly tells if the package should be installed at the user site or not.

@McSinyx
Copy link
Contributor

McSinyx commented May 19, 2020 •
edited

There's get_lib_location_guesses in the same file, which derives from the distutils' scheme (see _internal.location to see the extract of it from distutils.Distribution). IMHO the concrete location is only decided after decide_user_install, which I'm having a PR reworking on (GH-7828, edit: the mention is to let you know that the logic rework shuffled up the order in decide_user_install).

@pradyunsg
Copy link
Member

It's probably better to figure out what to do for the install location logging in a follow up PR.

deveshks
deveshks reviewed May 20, 2020
View reviewed changes
src/pip/_internal/commands/install.py
@@ -249,6 +250,7 @@ def run(self, options, args):

install_options = options.install_options or []

logger.debug("Using {}".format(get_pip_version()))
Copy link
Contributor

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.

@gutsytechster
Copy link
Contributor Author

@McSinyx, AFAICU, the get_lib_location_guesses returns the purelib and platlib values which can be same or different based on the system's OS. And disutils_scheme gives output similar to

{'purelib': '/home/gutsytechster/miniconda3/envs/pip-env/lib/python3.8/site-packages', 'platlib': '/home/gutsytechster/miniconda3/envs/pip-env/lib/python3.8/site-packages', 'headers': '/home/gutsytechster/miniconda3/envs/pip-env/include/python3.8/UNKNOWN', 'scripts': '/home/gutsytechster/miniconda3/envs/pip-env/bin', 'data': '/home/gutsytechster/miniconda3/envs/pip-env'}

However, I still don't get how we can find the concrete location! 😐

@McSinyx
Copy link
Contributor

McSinyx commented May 20, 2020

@gutsytechster, IIUC these are concrete locations (purelib for pure Python, platlib for extensions). I think you can just move the assignment of lib_locations after L275.

@BrownTruck
Copy link
Contributor

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 23, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 23, 2020
pradyunsg
pradyunsg reviewed May 23, 2020
View reviewed changes
news/3166.feature Outdated Show resolved Hide resolved
xavfernandez
xavfernandez reviewed Jun 9, 2020
View reviewed changes
tests/functional/test_install.py Outdated


def test_install_logs_pip_version_in_debug(script):
result = script.pip('install', '-v', 'INITools==0.2')
Copy link
Member

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.

@BrownTruck
Copy link
Contributor

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 6, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 9, 2020
@gutsytechster gutsytechster reopened this Jul 10, 2020
@chrahunt chrahunt merged commit 15b7afb into pypa:master Jul 10, 2020
@chrahunt
Copy link
Member

Thanks @gutsytechster!

@gutsytechster gutsytechster deleted the add_logs_for_install branch July 22, 2020 10:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

@deveshks deveshks deveshks left review comments

@chrahunt chrahunt chrahunt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@gutsytechster @pradyunsg @McSinyx @BrownTruck @chrahunt @xavfernandez @deveshks @pypa-bot