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

Change VCS tooling verbosity along with pip's verbosity #9639

Merged
merged 11 commits into from Jan 25, 2022

Conversation

SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Feb 20, 2021

Overview

This PR is responding to (part of) the feature request represented by #8819, passing the verbosity of pip down to a VCS used to satisfy a requirement when calling fetch_new() (i.e. executing a clone).

I haven't contributed to pip before, so I'm opening this as a WIP to get some feedback on my changes to the internal plumbing, namely how verbose is passed down to the VersionControl instance. It seems like there are probably other ways to approach this problem, so I figured it's better to just ask about what I've done for git alone before proceeding to the other backends, in case there is some better way to pass this information down to the tool level.

Only the set of changes necessary to get the desired behavior when installing from a git repository is included at the moment.

Note: I discovered this feature request because of a user who wanted to see the actual progress of the clone, which requires passing --progress to git when not attached to a terminal. I don't think this is essential to the reporting ticket, but it's nice to have and produced reasonable output, so I've included it in these changes.

Output

Truncated output of typical usage follows:

default:

$ python3 -m pip install --target /tmp/otherpip git+https://github.com/pypa/pip
Collecting git+https://github.com/pypa/pip
  Cloning https://github.com/pypa/pip to /tmp/pip-req-build-i3w858wj
  Running command git clone --quiet https://github.com/pypa/pip /tmp/pip-req-build-i3w858wj
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
    Preparing wheel metadata: started
    Preparing wheel metadata: finished with status 'done'
Building wheels for collected packages: pip
  Building wheel for pip (PEP 517): started
  Building wheel for pip (PEP 517): finished with status 'done'
  Created wheel for pip: filename=pip-21.1.dev0-py3-none-any.whl size=1522347 sha256=67fcfe60e62d64239ec2725e7233db2f25d5d52fc00d781e006becc53b
  Stored in directory: /tmp/pip-ephem-wheel-cache-7eghaoih/wheels/48/9d/74/8de2038273187621570de5b4d06275d64086de14c84da727ff
Successfully built pip
Installing collected packages: pip
Successfully installed pip-21.1.dev0

verbose:

$ python3 -m pip -vvv install --target /tmp/otherpip git+https://github.com/pypa/pip
Using pip 21.1.dev0 from /home/snoopjedi/repos/pip-src/src/pip (python 3.6)
Non-user install due to --prefix or --target option
Created temporary directory: /tmp/pip-target-dherg1bq
Created temporary directory: /tmp/pip-ephem-wheel-cache-4fvg0opc
Created temporary directory: /tmp/pip-req-tracker-wtcxtdds
Initialized build tracking at /tmp/pip-req-tracker-wtcxtdds
Created build tracker: /tmp/pip-req-tracker-wtcxtdds
Entered build tracker: /tmp/pip-req-tracker-wtcxtdds
Created temporary directory: /tmp/pip-install-n4r1uosw
Collecting git+https://github.com/pypa/pip
  Created temporary directory: /tmp/pip-req-build-m1u4sukt
  Cloning https://github.com/pypa/pip to /tmp/pip-req-build-m1u4sukt
  Running command git clone --verbose --progress https://github.com/pypa/pip /tmp/pip-req-build-m1u4sukt
  Cloning into '/tmp/pip-req-build-m1u4sukt'...
  POST git-upload-pack (gzip 5065 to 2595 bytes)
  remote: Enumerating objects: 53, done.
  remote: Counting objects: 100% (53/53), done.
  remote: Compressing objects: 100% (44/44), done.
  remote: Total 71289 (delta 17), reused 24 (delta 7), pack-reused 71236
  Receiving objects: 100% (71289/71289), 59.23 MiB | 22.36 MiB/s, done.
  Resolving deltas: 100% (47677/47677), done.
  Added git+https://github.com/pypa/pip to build tracker '/tmp/pip-req-tracker-wtcxtdds'
  Created temporary directory: /tmp/pip-build-env-8y3yii2n
  Installing build dependencies: started

...verbose build output...

Successfully built pip
Installing collected packages: pip
  Running command git rev-parse HEAD
  8a949a1c52ae47584242e5bb0155d1d1018275ae

  Creating /tmp/pip-target-dherg1bq/bin
  changing mode of /tmp/pip-target-dherg1bq/bin/pip to 775
  changing mode of /tmp/pip-target-dherg1bq/bin/pip3 to 775
  changing mode of /tmp/pip-target-dherg1bq/bin/pip3.6 to 775
Successfully installed pip-21.1.dev0

TODO

  • Pass pip test suite
  • Needs news fragment

@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 Feb 24, 2021
@pradyunsg
Copy link
Member

@SnoopJeDi Are you still interested in moving this forward? If so, could you update this PR and resolve the merge conflicts?

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Sep 18, 2021
@SnoopJ
Copy link
Contributor Author

SnoopJ commented Sep 20, 2021

@SnoopJeDi Are you still interested in moving this forward? If so, could you update this PR and resolve the merge conflicts?

@pradyunsg Yes and yes. Will do, thanks for following up.

Can you comment on the question I asked about the plumbing changes? I.e. is it okay to add a verbose kwarg to the relevant functions to pass that info down to where it's needed to couple it to the VCS behavior? I don't know the pip codebase well enough to know if there's some nicer way to access config-at-invocation information like this, but didn't find one when I wrote the PR.

Summary of the changed signatures, most abstract to least:

# src/pip/_internal/operations/prepare.py
unpack_vcs_link(link, location) → unpack_vcs_link(link, location, verbose)

# src/pip/_internal/vcs/versioncontrol.py
obtain(self, dest, url) → obtain(self, dest, url, verbose)
unpack(self, location, url) → unpack(self, location, url, verbose)

# src/pip/_internal/vcs/git.py
fetch_new(self, dest, url, rev_options) → fetch_new(self, dest, url, rev_options, verbose)

@ziebam
Copy link
Contributor

ziebam commented Sep 23, 2021

FWIW I peaked around the codebase and didn't find a nicer way to access information like that either. I think @pradyunsg might have been fine with the changes so far and that's why they asked for the rebase, but I'm not exactly sure.

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 6, 2021
@uranusjr
Copy link
Member

uranusjr commented Oct 6, 2021

Is this still in WIP state? I'm waiting this to be considered ready before taking a look at the implementation.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Oct 6, 2021

Is this still in WIP state? I'm waiting this to be considered ready before taking a look at the implementation.

@uranusjr yes, this PR is still WIP, it's missing implementation for non-git VCS.

@pfmoore
Copy link
Member

pfmoore commented Oct 6, 2021

FWIW, I don't think this needs to block on deciding the perfect way of passing the "verbose" flag. The current proposal to change just the one signature unpack_vcs_link outside of the VCS support code seems clean enough for now, and we can always refactor later if needed.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Oct 6, 2021

FWIW, I don't think this needs to block on deciding the perfect way of passing the "verbose" flag. The current proposal to change just the one signature unpack_vcs_link outside of the VCS support code seems clean enough for now, and we can always refactor later if needed.

Agreed, when I come back for the other VCS implementations, I'll be using this approach.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Oct 11, 2021

Failing tests as of 3042def appear to all be of the form:

TypeError: obtain() missing 1 required positional argument: 'verbose'

Which should be simple to resolve. @uranusjr this is no longer WIP as far as implementation is concerned.

@SnoopJ SnoopJ changed the title WIP: Pass pip verbosity to fetch_new() in VCS tooling (#8819) Pass pip verbosity to fetch_new() in VCS tooling (#8819) Oct 11, 2021
@uranusjr
Copy link
Member

Logic lgtm, just needs to fix the linter errors and add a news entry.

news/8819.feature.rst Outdated Show resolved Hide resolved
@DiddiLeija DiddiLeija removed the S: awaiting response Waiting for a response/more information label Oct 20, 2021
@uranusjr uranusjr added this to the 22.0 milestone Oct 21, 2021
@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Nov 19, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 16, 2022
@SnoopJ
Copy link
Contributor Author

SnoopJ commented Jan 21, 2022

Current CI failure is due to a docs change introduced by #9442

@pradyunsg
Copy link
Member

See #10813

@pradyunsg pradyunsg self-assigned this Jan 25, 2022
@pradyunsg pradyunsg changed the title Pass pip verbosity to fetch_new() in VCS tooling (#8819) Change VCS tooling verbosity along with pip's verbosity Jan 25, 2022
@pradyunsg pradyunsg merged commit 5c24a79 into pypa:main Jan 25, 2022
@pradyunsg
Copy link
Member

Thanks @SnoopJeDi for your work on this, as well as your patience working through this with us! ^>^

@SnoopJ SnoopJ deleted the track-pip-verbosity-in-VCS branch January 25, 2022 16:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants