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

feat(install): add 'exact' option #5963

Closed
wants to merge 1 commit into from
Closed

feat(install): add 'exact' option #5963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 pipenv/cli/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ def install(state, **kwargs):
python=state.python,
pypi_mirror=state.pypi_mirror,
system=state.system,
exact=state.installstate.exact,
ignore_pipfile=state.installstate.ignore_pipfile,
requirementstxt=state.installstate.requirementstxt,
pre=state.installstate.pre,
Expand Down
20 changes: 20 additions & 0 deletions pipenv/cli/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class InstallState:
def __init__(self):
self.dev = False
self.pre = False
self.exact = False
self.ignore_pipfile = False
self.code = False
self.requirementstxt = None
Expand Down Expand Up @@ -130,6 +131,24 @@ def callback(ctx, param, value):
)(f)


def exact_option(f):
def callback(ctx, param, value):
state = ctx.ensure_object(State)
state.installstate.exact = value
return value

return option(
"--exact",
is_flag=True,
default=False,
expose_value=False,
help="Add exact package version to Pipfile when installing, instead of *.",
callback=callback,
type=click_types.BOOL,
show_envvar=True,
)(f)


def ignore_pipfile_option(f):
def callback(ctx, param, value):
state = ctx.ensure_object(State)
Expand Down Expand Up @@ -607,6 +626,7 @@ def install_options(f):
f = sync_options(f)
f = index_option(f)
f = requirementstxt_option(f)
f = exact_option(f)
f = ignore_pipfile_option(f)
f = editable_option(f)
f = package_arg(f)
Expand Down
12 changes: 11 additions & 1 deletion pipenv/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,13 +1197,23 @@ def generate_package_pipfile_entry(self, package, pip_line, category=None):
else:
return name, normalized_name, entry

def add_package_to_pipfile(self, package, pip_line, dev=False, category=None):
def add_package_to_pipfile(
self, package, pip_line, dev=False, category=None, exact=False
):
category = category if category else "dev-packages" if dev else "packages"

name, normalized_name, entry = self.generate_package_pipfile_entry(
package, pip_line, category=category
)

if exact:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could get this from the actual full resolver call rather than do a 1-off resolution of just this package, because the version may end up being different if another package constraints it. Just from looking a bit at the code again, I'll note the lock resolution and updating the lock file already happened prior to this point inside of do_init -- We could just read the version of that package from the lockfile for the happy path, and then perhaps handle the --skip-lock case the way you are describing below (since that flag bypasses the lock phase and lock file).

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, that is a valid concern. Let me see if I can figure it out.

Copy link
Author

@cansin cansin Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

After a quick look, I think do_init (pipenv.routines.install:286) happens after add_package_to_pipfile (pipenv.routines.install:246&260) call. Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

... get this from the actual full resolver call rather than do a 1-off resolution of just this package, because the version may end up being different if another package constraints it. ...

^ But I think you are right about the above concern, so this PR might be a no-go, as is.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think you are right now -- its a catch-22 because you need to be able to resolve against something to get the result to know what specifier you want to exactly pin to -- its probably a case of:
1.) Leave the Pipfile entry alone at this step
2.) Let the lock resolution and lock file update happen
3.) Amend the Pipfile entry after that, likely reading from the lock file for the specifier. (This is the hard part, it would need to happen post-lock but somewhere that has knowledge of the newly added package from the CLI args).

Copy link
Author

Choose a reason for hiding this comment

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

Or perhaps separate pip list update, pipfile update, lock resolution and lock file update so that the order could become:

  1. Update the list of pip packages,
  2. Update the lock resolution,
  3. Update the Pipfile,
  4. Update the Pipfile.lock.

I think the main challenge right now is 1 and 3, as well as 2 and 4 is coupled so that it is not easy to introduce an intermediary step. If they are to be separated one could:

  1. Update the list of pip packages,
  2. Update the lock resolution,
  3. Update the list of pip packages yet again to fixate the version for the new package,
  4. Update the Pipfile,
  5. Update the Pipfile.lock.

But I do not know how easy of a refactor that would be.

Copy link
Author

Choose a reason for hiding this comment

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

I guess then again, writing to file is not "that" heavy, and we might not need to worry about it. My worry is, when the version of the package at the Pipfile changes, would that somehow affect the Pipfile.lock content? For instance, I believe it'd on npm as afaik they do type out both the requested version and the resolved version to their lock file.

Copy link
Author

@cansin cansin Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If that is the case the order would become:

  1. Update the list of pip packages,
  2. Update the lock resolution,
  3. Update the list of pip packages yet again to fixate the version for the new package,
  4. Update the lock resolution again,
  5. Update the Pipfile,
  6. Update the Pipfile.lock.

Copy link
Member

@matteius matteius Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

when the version of the package at the Pipfile changes, would that somehow affect the Pipfile.lock content?

Ah that is a good call out -- the meta hash in the lock file is basically a hash of the Pipfile content -- if the Pipfile content changes after the lockfile is updated, the meta-hash would need to be re-set as well. That would be about it though, the * specifier would get used for resolution, and then set to the exact version from the resolution results.

Just going to restate the steps you outlined with some notes:

  1. Update the list of pip packages, (update the list of pacakges to resolve based on Pipfile specifiers)
  2. Perform the lock resolution using the complete list of specifiers (new package from CLI defaults to * as today)
  3. Update the list of pip packages yet again to fixate the version for the new package, (This really just needs to be done in the Pipfile at some point using the lock resolution results)
  4. Update the lock resolution again, (I don't think this is necessary because the first lock resolution would have had everything specified and the result of that is what we know to be the exact version of the CLI package)
  5. Update the Pipfile, (Maybe not necessary if we already had done this).
  6. Update the Pipfile.lock. Yes -- if we update the Pipfile before we update the lock with the resolution results, then there is no mucking around with re-updating the meta hash. I still content the original resolution phase though has all the information we need for normal install and no reason to try two resolution phases.

from pipenv.utils.resolver import resolve_deps

resolved_packages, resolver = resolve_deps(
{normalized_name: package.name}, None, self, category=category
)
entry = str(resolver.install_reqs[normalized_name].specifier)

return self.add_pipfile_entry_to_pipfile(
name, normalized_name, entry, category=category
)
Expand Down
9 changes: 7 additions & 2 deletions pipenv/routines/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ def do_install(
python=False,
pypi_mirror=None,
system=False,
exact=False,
ignore_pipfile=False,
requirementstxt=False,
pre=False,
Expand Down Expand Up @@ -243,7 +244,11 @@ def do_install(
if categories:
for category in categories:
added, cat, normalized_name = project.add_package_to_pipfile(
pkg_requirement, pkg_line, dev, category
pkg_requirement,
pkg_line,
dev,
category=category,
exact=exact,
)
if added:
new_packages.append((normalized_name, cat))
Expand All @@ -253,7 +258,7 @@ def do_install(
)
else:
added, cat, normalized_name = project.add_package_to_pipfile(
pkg_requirement, pkg_line, dev
pkg_requirement, pkg_line, dev, exact=exact
)
if added:
new_packages.append((normalized_name, cat))
Expand Down