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

install should not do full lock resolution, as that is the purpose of pipenv lock #6276

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

matteius
Copy link
Member

@matteius matteius commented Oct 18, 2024

The issue and fixes

Fixes #6267

Possibly fixes #6231
Possibly fixes #6187
Possibly fixes #6068

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 #.

"colorama",
"atomicwrites",
"six",
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be acceptable because its the default non-sorting case; it is technically a behavioral change that re-installing something moves it to the bottom of the Pipfile, but I feel we get closed to reducing some complexity around the Pipfile creation, and writing in this PR -- it doesn't seem to negatively impact performance either.

@@ -379,9 +379,7 @@ def test_system_and_deploy_work(pipenv_instance_private_pypi):
@pytest.mark.basic
@pytest.mark.install
def test_install_creates_pipfile(pipenv_instance_pypi):
with pipenv_instance_pypi() as p:
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to debug this test, and I figure may as well use this simplification that existed in the fixture to not create the pipfile in the first place.

@pytest.mark.skipif(
sys.version_info >= (3, 12), reason="Package does not work with Python 3.12"
)
@pytest.mark.install
Copy link
Member Author

Choose a reason for hiding this comment

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

Upgrading this package meant the test could pass on python 3.13 even

def test_install_remote_wheel_file_with_extras(pipenv_instance_pypi):
with pipenv_instance_pypi() as p:
c = p.pipenv(
"install fastapi[dev]@https://files.pythonhosted.org/packages/4e/1a/04887c641b67e6649bde845b9a631f73a7abfbe3afda83957e09b95d88eb/fastapi-0.95.2-py3-none-any.whl"
"install -v fastapi[standard]@https://files.pythonhosted.org/packages/c9/14/bbe7776356ef01f830f8085ca3ac2aea59c73727b6ffaa757abeb7d2900b/fastapi-0.115.2-py3-none-any.whl"
Copy link
Member Author

Choose a reason for hiding this comment

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

dev is no longer an extra in newer fastapi versions.

@@ -143,9 +143,6 @@ def test_install_named_index_alias(pipenv_instance_private_pypi):
@pytest.mark.index
@pytest.mark.install
@pytest.mark.needs_internet
@pytest.mark.skipif(
sys.version_info >= (3, 12), reason="Package does not work with Python 3.12"
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was failing at one point, and I discovered that it could pass on 3.13 -- I think we had a different bug at one point that lead to this.

@@ -5,7 +5,7 @@
import sys
import tempfile
import warnings
from functools import cached_property, lru_cache
from functools import lru_cache
Copy link
Member Author

Choose a reason for hiding this comment

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

Too many side-effects encountered in various issue reports relate to our current structure and usage of caching. I am in favor of correcting core functionality, and revisiting small gaps in performance down the road in a more thoughtful way.

@@ -1132,6 +1132,11 @@ def install_req_from_pipfile(name, pipfile):
version = ""
req_str = f"{name}{extras_str}{version}"

# Handle markers before constructing InstallRequirement
Copy link
Member Author

Choose a reason for hiding this comment

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

AI suggested this change -- awaiting confirmation it fixes relevant issues.

@@ -164,7 +160,7 @@ def upgrade(
which=project._which,
project=project,
lockfile={},
category="default",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was definitely a bug in the update/upgrade path, it couldn't have worked too great for non-default categories.

@@ -45,13 +45,11 @@ def do_sync(
do_init(
project,
allow_global=system,
requirements_dir=requirements_dir,
Copy link
Member Author

Choose a reason for hiding this comment

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

Simplifying interface

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