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

use aggressive forking when encountering markers #5733

Closed
wants to merge 11 commits into from

Commits on Aug 9, 2024

  1. Configuration menu
    Copy the full SHA
    44ed1c9 View commit details
    Browse the repository at this point in the history
  2. uv/tests: update snapshot that forks when it previously didn't

    Previously, since the markers weren't disjoint, this didn't result in a
    fork. But now that we deal with overlapping markers, this is fine. And
    seems correct.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    04a62cf View commit details
    Browse the repository at this point in the history
  3. uv/tests: only consider dependency specification for fork matching ma…

    …rker
    
    The test in this case has this comment:
    
    ```
    /// If a dependency requests a prerelease version with an overlapping marker expression,
    /// we should prefer the prerelease version in both forks.
    ```
    
    With this setup:
    
    ```
        let pyproject_toml = context.temp_dir.child("pyproject.toml");
        pyproject_toml.write_str(indoc! {r#"
            [project]
            name = "example"
            version = "0.0.0"
            dependencies = [
                "cffi >= 1.17.0rc1 ; os_name == 'Linux'"
            ]
            requires-python = ">=3.11"
        "#})?;
    
        let requirements_in = context.temp_dir.child("requirements.in");
        requirements_in.write_str(indoc! {"
            cffi
            .
        "})?;
    ```
    
    The change in this commit _seems_ more correct that what we had,
    although it does seem to contradict the comment. Namely, in the `os_name
    != "Linux"` fork, we don't prefer the pre-release version since the
    `cffi >= 1.17.0rc1` bound doesn't apply.
    
    It's not quite clear what to do in this instance.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    a18ccf2 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    cf09206 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    3d18567 View commit details
    Browse the repository at this point in the history
  6. uv/tests: accept tests that now find a resolution which previously di…

    …dn't!
    
    I wasn't expecting these scenarios to suddenly work, but because of the
    aggressive forking strategy of overlapping markers, these both now
    produce a valid resolution. Previously, we *only* forked when there were
    disjoint markers for the same dependency specification. But now we
    potentially fork whenever there is any single marker, and this results
    in different forks reaching what were conflicting dependency
    specifications. And thus, things work here.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    e5a4fd1 View commit details
    Browse the repository at this point in the history
  7. uv/tests: update preferences test

    From looking at this carefully, I believe the result here is still
    correct but "different" from the status quo based on the order in which
    forks are processed. (At time of writing, we do sort forks, but we don't
    use comprehensive criteria. So at least in some cases, the order depends
    on the order that the forks were created. Since the overlapping markers
    code completely changes how we generate forks, that seems plausible
    here.)
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    2872c4c View commit details
    Browse the repository at this point in the history
  8. uv/tests: update error message that suggests a fork

    This is an interesting update that previously used a message without a
    fork occurring, but now notes a split. The dependencies in this case
    are:
    
        dependencies = [
          '''fork-marker-disjoint-a>=2; sys_platform == "linux"''',
          '''fork-marker-disjoint-a<2; sys_platform == "linux"''',
        ]
    
    In this case, the markers are the same for conflicting dependency
    specifications. So the "no resolution" result is correct. But
    previously, we wouldn't fork here, because we wouldn't consider the
    marker expressions to be disjoint.
    
    But with overlapping markers, we are actually forking here. Namely,
    we consider a fork with `sys_platform == "linux"` and another with
    `sys_platform != "linux"`.
    
    I actually don't think this is even related to overlapping markers. The
    previous behavior seems not quite right (even though it led to the same
    result), because it implies we weren't really considering the "linux"
    and "not linux" cases as distinct in resolution.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    0495d72 View commit details
    Browse the repository at this point in the history
  9. uv/tests: fix for 'pysocks' optional dependency for 'requests'

    This comes from this dependency specification:
    
        dependencies = ["requests", "requests[socks] ; python_version < '3.10'"]
    
    Previously, the `pysocks` dependency was unconditionally included when
    the `socks` extra was enabled. But it should only be included when
    `python_version < '3.10'`, which is now reflected here via a marker.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    462420f View commit details
    Browse the repository at this point in the history
  10. uv/tests: update snapshots from overlapping marker change

    This commit is meant to group together test updates that are
    "uninteresting." That is, most updates are just adding marker
    expressions that are "always true" or establishing the forks in the
    lock file.
    
    I've tried to split out the interesting snapshot changes into subsequent
    commits.
    
    Some of the marker changes here are quite striking. I checked most of
    them. Every one I checked, including the big ones, are all just fancy
    ways of saying, "always true."
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    1aa62a1 View commit details
    Browse the repository at this point in the history
  11. uv/tests: WRONG test changes

    These changes all look wrong and would need to be resolved before
    merging.
    BurntSushi committed Aug 9, 2024
    Configuration menu
    Copy the full SHA
    23832ff View commit details
    Browse the repository at this point in the history