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

Remove dead code related to InstallRequirement build directories #7405

Merged
merged 3 commits into from
Nov 27, 2019

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Nov 26, 2019

Going back a few years, InstallRequirement._ideal_build_dir has been a string. As a result, the second half of move_to_correct_build_directory couldn't've been executed, because it was asserting self._ideal_build_dir.path, which is not an attribute on strings.

See:

  1. self._ideal_build_dir is only set in InstallRequirement.ensure_build_location(build_dir), to the value of the build_dir parameter
  2. Calls to InstallRequirement.ensure_build_location(build_dir):
    1. From move_to_correct_build_directory itself, but we can leave that aside as it is after the invalid assertion in question
    2. From WheelBuilder.build with RequirementPreparer.build_dir which is set to a string here
    3. From InstallRequirement.ensure_has_source_dir which itself is only called from
      1. RequirementPreparer.prepare_linked_requirement here with self.build_dir (already established as a string)
      2. RequirementPreparer.prepare_editable_requirement here with self.src_dir, which is a string per
        1. construction with options.src_dir here, the value of which is only ever a string per the only locations it is set
          1. DownloadCommand.run
          2. WheelCommand.run
          3. InstallCommand.run

Since self._ideal_build_dir is only either None or a string, we must
have always been returning from this function.

This goes back to pip 10.0.0, so this code has been dead for some time.
@chrahunt chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Nov 26, 2019
@chrahunt chrahunt marked this pull request as ready for review November 27, 2019 00:24
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Nice catch 👍

@pradyunsg
Copy link
Member

Hah! I was gonna file a similar PR -- you beat me to it. :)

@pradyunsg pradyunsg merged commit 2c9ee4a into pypa:master Nov 27, 2019
@chrahunt chrahunt deleted the maint/refactor-install-req-dirs branch November 27, 2019 22:57
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 28, 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 skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants