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

Don't check against root directory #4275

Merged
merged 6 commits into from
May 29, 2020
Merged

Don't check against root directory #4275

merged 6 commits into from
May 29, 2020

Conversation

frostming
Copy link
Contributor

@frostming frostming commented May 28, 2020 •
edited
Loading

The issue

#3386 causes some regression issue #4273

The fix

This PR reverts some of the changes of #3386, don't check against root directory but fix the self.name reference in create_pipfile().

Sorry for the trouble.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@frostming frostming added Priority: High This item is high priority and should be resolved quickly. Type: Regression This issue is a regression of a previous behavior. labels May 28, 2020
@techalchemy
Copy link
Member

The main reason for the change in the first place is due to locating virtual environments & related python paths -- as far as I was aware, this was a substantial cause of bugs and breakages and basically didn't work. The fact that it is breaking workflows is the first I am hearing of it working at all.

@techalchemy techalchemy mentioned this pull request May 28, 2020
Closed
jayaddison
jayaddison reviewed May 28, 2020
View reviewed changes
pipenv/project.py Outdated Show resolved Hide resolved
techalchemy
techalchemy requested changes May 29, 2020
View reviewed changes
Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

One small update to how we are determining extra index url defaults -- our syntax here is kind of outdated

pipenv/project.py Outdated Show resolved Hide resolved
pipenv/project.py Outdated Show resolved Hide resolved
@techalchemy
Copy link
Member

I think the changes make sense overall -- my only question is -- wasn't one of our main issues regarding the which functionality that is partly derived from project locations? If that is completely resolved (which it should be, I would think) then we shouldn't have any issues allowing pipenv usage from /

techalchemy
techalchemy reviewed May 29, 2020
View reviewed changes
pipenv/project.py Outdated Show resolved Hide resolved
techalchemy
techalchemy reviewed May 29, 2020
View reviewed changes
pipenv/project.py Outdated Show resolved Hide resolved
@frostming
Copy link
Contributor Author

One small update to how we are determining extra index url defaults -- our syntax here is kind of outdated

Agree, do as suggested

my only question is -- wasn't one of our main issues regarding the which functionality that is partly derived from project locations? If that is completely resolved (which it should be, I would think) then we shouldn't have any issues allowing pipenv usage from /

Yes, when under the root directory self.name is just an empty string, which can work in subsequent location calculations.

pipenv/pipenv/project.py

Lines 220 to 224 in 3f4abad

@property
def name(self):
if self._name is None:
self._name = self.pipfile_location.split(os.sep)[-2]
return self._name

@frostming frostming linked an issue May 29, 2020 that may be closed by this pull request
Pipenv no longer working under the root directory is not listed as a breaking change #4273
Closed
techalchemy
techalchemy approved these changes May 29, 2020
View reviewed changes
Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

This is ok with me, unless you want to try some alternative approach for the cache dir removal... but it's less important to clean up pip's cache anyhow

pipenv/core.py Outdated
except OSError as e:
# Ignore FileNotFoundError. This is needed for Python 2.7.
import errno

if e.errno == errno.ENOENT:
pass
raise
# Other processes may be writing into this directory simultaneously.
vistir.path.rmtree(locations.USER_CACHE_DIR, ignore_errors=True)
Copy link
Member

Choose a reason for hiding this comment

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

yeah i guess that's fine, or maybe ignore_errors=environments.PIPENV_IS_CI

Copy link
Member

Choose a reason for hiding this comment

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

(but I'm not sure that's necessary, and don't delay merging over it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that env var, that looks better.

@frostming frostming merged commit a17a9ab into master May 29, 2020
@frostming frostming deleted the bugfix/4273 branch May 29, 2020 09:25
@techalchemy techalchemy added this to the 2020.6.x bugfix release milestone Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayaddison jayaddison jayaddison left review comments

@techalchemy techalchemy techalchemy approved these changes

Assignees
No one assigned
Labels
Priority: High This item is high priority and should be resolved quickly. Type: Regression This issue is a regression of a previous behavior.
Projects
None yet
Milestone
2020.6.x bugfix release
Development

Successfully merging this pull request may close these issues.

Pipenv no longer working under the root directory is not listed as a breaking change
3 participants
@frostming @techalchemy @jayaddison