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

Decouple project from the existence of Pipfile. #3386

Merged
merged 5 commits into from
Apr 1, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/3386.trivial.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Decouple project from the existence of Pipfile.
frostming marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 5 additions & 1 deletion pipenv/cli/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,11 @@ def cli(
# There is no virtualenv yet.
if not project.virtualenv_exists:
echo(
crayons.red("No virtualenv has been created for this project yet!"),
"{}({}){}".format(
crayons.red("No virtualenv has been created for this project"),
crayons.white(project.project_directory, bold=True),
crayons.red(" yet!")
),
err=True,
)
ctx.abort()
Expand Down
27 changes: 16 additions & 11 deletions pipenv/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,11 +540,16 @@ def ensure_project(
# Automatically use an activated virtualenv.
if PIPENV_USE_SYSTEM:
system = True
if not project.pipfile_exists:
if deploy is True:
raise exceptions.PipfileNotFound
else:
project.touch_pipfile()
Copy link
Member

Choose a reason for hiding this comment

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

We do still want to create non existent pipfiles when we ensure projects exist, no?

if not project.pipfile_exists and deploy:
raise exceptions.PipfileNotFound
# Fail if working under /
if not project.name:
click.echo(
"{0}: Pipenv is not intended to work under the root directory, "
"please choose another path.".format(crayons.red("ERROR")),
err=True
)
sys.exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't touch an empty Pipfile here.

# Skip virtualenv creation when --system was used.
if not system:
ensure_virtualenv(
Expand Down Expand Up @@ -607,24 +612,24 @@ def shorten_path(location, bold=False):
def do_where(virtualenv=False, bare=True):
"""Executes the where functionality."""
if not virtualenv:
location = project.pipfile_location
# Shorten the virtual display of the path to the virtualenv.
if not bare:
location = shorten_path(location)
if not location:
if not project.pipfile_exists:
Copy link
Member

Choose a reason for hiding this comment

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

So this isn’t different fundamentally. pipfile_exists is just bool(pipfile_location). It is arguably cleaner so it seems ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pipfile_location no longer returns None, checking the existence makes more sense.

click.echo(
"No Pipfile present at project home. Consider running "
"{0} first to automatically generate a Pipfile for you."
"".format(crayons.green("`pipenv install`")),
err=True,
)
return
location = project.pipfile_location
# Shorten the virtual display of the path to the virtualenv.
if not bare:
location = shorten_path(location)
Copy link
Member

Choose a reason for hiding this comment

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

if not bare...elif not bare seems like it might be a mistake

elif not bare:
click.echo(
"Pipfile found at {0}.\n Considering this to be the project home."
"".format(crayons.green(location)),
err=True,
)
pass
else:
click.echo(project.project_directory)
else:
Expand Down
25 changes: 8 additions & 17 deletions pipenv/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def _normalized(p):
path_str = matches and matches[0] or str(loc)
else:
path_str = str(loc)
return normalize_drive(path_str)
return normalize_drive(os.path.abspath(path_str))
Copy link
Member

Choose a reason for hiding this comment

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

Don’t change this one, it is going to regress environment discovery on windows unless you change every reference to it

Copy link
Contributor Author

@frostming frostming Dec 19, 2018

Choose a reason for hiding this comment

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



DEFAULT_NEWLINES = u"\n"
Expand Down Expand Up @@ -222,7 +222,7 @@ def name(self):

@property
def pipfile_exists(self):
return bool(self.pipfile_location)
return os.path.isfile(self.pipfile_location)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the existence of the file instead

Copy link
Member

Choose a reason for hiding this comment

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

Don’t use isfile. I know it seems intuitive but this will break pipenv for lots of people. Changing things like this en mass for no reason isn’t going to fly. This library is downloaded millions of times a month. Even if 1% of the users set their Pipfile to a network path or use the envvar to point at a temp file that doesn’t exist to work around some issue, you just fundamentally broke pipenv for those users because of this change.

Improvements are good and important and I’m willing to break compatibility for some things. But changes like this one only screw users over and make people like Hynek write about how many regressions they experience

Copy link
Contributor Author

@frostming frostming Dec 19, 2018

Choose a reason for hiding this comment

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

This change is made to work with #3386 (diff)


@property
def required_python_version(self):
Expand All @@ -237,11 +237,7 @@ def required_python_version(self):

@property
def project_directory(self):
if self.pipfile_location is not None:
return os.path.abspath(os.path.join(self.pipfile_location, os.pardir))

else:
return None
return os.path.abspath(os.path.join(self.pipfile_location, os.pardir))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return the project home even if there is no Pipfile existing.

NOTE: $ pipenv --where still prints an error if Pipfile doesn't exist.


@property
def requirements_exists(self):
Expand All @@ -255,8 +251,7 @@ def is_venv_in_project(self):

@property
def virtualenv_exists(self):
# TODO: Decouple project from existence of Pipfile.
if self.pipfile_exists and os.path.exists(self.virtualenv_location):
if os.path.exists(self.virtualenv_location):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So pipenv --venv and pipenv --rm is possible when Pipfile is not present.

if os.name == "nt":
extra = ["Scripts", "activate.bat"]
else:
Expand Down Expand Up @@ -470,7 +465,7 @@ def pipfile_location(self):
try:
loc = pipfile.Pipfile.find(max_depth=PIPENV_MAX_DEPTH)
except RuntimeError:
loc = None
loc = "Pipfile"
Copy link
Member

Choose a reason for hiding this comment

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

I’m pretty sure this returns an actual path like project.path_to("Pipfile")

Copy link
Contributor Author

@frostming frostming Dec 19, 2018

Choose a reason for hiding this comment

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

In case the Pipfile is not found it returns None. Then https://github.com/pypa/pipenv/blob/master/pipenv/project.py#L217-L221 will be broken.

self._pipfile_location = _normalized(loc)
return self._pipfile_location

Expand Down Expand Up @@ -499,6 +494,8 @@ def parsed_pipfile(self):

def read_pipfile(self):
# Open the pipfile, read it into memory.
if not self.pipfile_exists:
return ""
with io.open(self.pipfile_location) as f:
contents = f.read()
self._pipfile_newlines = preferred_newlines(f)
Expand Down Expand Up @@ -659,11 +656,6 @@ def dev_packages(self):
"""Returns a list of dev-packages, for pip-tools to consume."""
return self._build_package_list("dev-packages")

def touch_pipfile(self):
"""Simply touches the Pipfile, for later use."""
with open("Pipfile", "a"):
os.utime("Pipfile", None)

@property
def pipfile_is_empty(self):
if not self.pipfile_exists:
Expand All @@ -680,7 +672,6 @@ def create_pipfile(self, python=None):
ConfigOptionParser, make_option_group, index_group
)

name = self.name if self.name is not None else "Pipfile"
config_parser = ConfigOptionParser(name=self.name)
config_parser.add_option_group(make_option_group(index_group, config_parser))
install = config_parser.option_groups[0]
Expand Down Expand Up @@ -834,7 +825,7 @@ def write_lockfile(self, content):

@property
def pipfile_sources(self):
if "source" not in self.parsed_pipfile:
if self.pipfile_is_empty or "source" not in self.parsed_pipfile:
return [DEFAULT_SOURCE]
# We need to make copies of the source info so we don't
# accidentally modify the cache. See #2100 where values are
Expand Down