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

Fix pip freeze not showing correct entry for mercurial packages that use subdirectories. #7072

Merged
merged 17 commits into from
Oct 12, 2019

Conversation

TonyBeswick
Copy link
Contributor

@TonyBeswick TonyBeswick commented Sep 24, 2019

fixes #7071

Changes are based on functionality that exists in the Git implementation, copied and modified for Mercurial.

Also changes the behavior of VcsSupport.controls_location() which now traverses up the filesystem looking for an appropriate vcs directory (.git, .hg etc) before invoking the applicable vcs command. This solves the problem of errors being printed to stderr due to vcs commands being run when no such vcs is installed. i.e. installing as editable from mercurial would cause pip freeze to call git and print an error if git is not installed.

TonyBeswick and others added 7 commits September 24, 2019 11:46
…so it porperly detects that subdirectories are under mercurial control.
…her the source or setup.py is located in a subdirectory of the repo root.
@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 Sep 25, 2019
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for the contribution.

This looks like a good approach, I have a few minor comments.

src/pip/_internal/vcs/git.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/mercurial.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/mercurial.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/mercurial.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/mercurial.py Outdated Show resolved Hide resolved
src/pip/_internal/vcs/mercurial.py Outdated Show resolved Hide resolved
tests/functional/test_freeze.py Outdated Show resolved Hide resolved
tbeswick-enphase and others added 2 commits October 8, 2019 18:15
…rial`, adding `get_repo_root_dir()` for the vcs specific code.

- Reverted behaviour of `Git.controls_location()` and `Mercurial.controls_location()` to call the vcs command if the base `VersionControl.controls_location()` doesn't detect the vcs directory.
- Added `log_failed_cmd` argument `VcsSupport.run_command()` to allow vcs commands to be tried without logging errors if they aren't present.
- Corrected indentation.
- Removed `expect_stderr=True` in `test_freeze_mercurial_clone_srcdir` as its not required.
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 8, 2019
…`Git.get_subdirectory()` and `Mercurial.get_subdirectory()`.

Added `find_path_to_setup_from_repo_root()` function to perform the
common parts of `get_subdirectory()`.
@chrahunt
Copy link
Member

It looks like LF got converted to CRLF on the latest commit.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! I think this is coming together nicely. Just two more comments.

src/pip/_internal/vcs/versioncontrol.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/subprocess.py Show resolved Hide resolved
…n optimization that belongs in another PR.
only got found after reverting `VersionControl.controls_location()`
@chrahunt chrahunt mentioned this pull request Oct 11, 2019
@xavfernandez
Copy link
Member

This has apparently conflicts with master: could you rebase it ?

@xavfernandez xavfernandez added this to the 19.3 milestone Oct 12, 2019
@pradyunsg pradyunsg merged commit 1c3f31c into pypa:master Oct 12, 2019
@pradyunsg
Copy link
Member

@xavfernandez I could merge this in, so I did. Lemme know if I did something wrong?

@xavfernandez
Copy link
Member

@xavfernandez I could merge this in, so I did. Lemme know if I did something wrong?

Strange, Github wasn't letting me merge it 🤔
Maybe a stale page...
But thank you for merging it :)

@TonyBeswick
Copy link
Contributor Author

Brilliant! Big thanks to @chrahunt for your eagle eyes and patience on this.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Nov 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip freeze doesn't show correct entry for mercurial packages that use subdirectories
7 participants