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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

cansin
Copy link

@cansin cansin commented Oct 3, 2023

The issue

Tries to very crudely address #5531 .

The fix

Introduces an --exact argument to pipenv install that locks the exact package version to Pipfile instead of *. It is a very rudimentary implementation and I am sure calling resolve_deps that early in the pipeline might introduce some unintended behavior. But I wanted to open up the PR to get some conversation going, and perhaps inspire someone else to implement a proper fix. Also to just say hi to @matteius I guess 馃憢

To-do

  • Write tests.
  • Add docs.
  • Update news/.

The checklist

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

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

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

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

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.

@matteius
Copy link
Member

matteius commented Oct 3, 2023

@cansin Really great hearing from you! I hope you have been happy and healthy. I did a double take when I saw the PR opened and thought well this is really cool! 馃槂

@cansin
Copy link
Author

cansin commented Oct 3, 2023

@matteius hey hey! I've been happy and most definitely healthier! Taking some sweet summertime back in Europe. Hope things are going great back home for you as well 馃憢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants