Repository: jazzband/pip-tools
Pull request #1582 (2022-02-14 23:35:38)

Direct references show extra requirements in .txt files
I am adding the missing extra requirements for direct references. I think it's better to keep consistency with how the rest of pip-tools works.

Refs #1613.

##### Contributor checklist

- [x] Provided the tests for the changes.
- [x] Assure PR title is short, clear, and good to be included in the user-oriented changelog

##### Maintainer checklist

- [x] Assure one of these labels is present: `backwards incompatible`, `feature`, `enhancement`, `deprecation`, `bug`, `dependency`, `docs` or `skip-changelog` as they determine changelog listing.
- [x] Assign the PR to an existing or new milestone for the target version (following [Semantic Versioning](https://blog.versioneye.com/2014/01/16/semantic-versioning/)).


AndydeCleyre: Thanks!

Yes, the extras should be included. A similar fix is sitting in #1329, not yet reviewed by @atugushev:

```python
    extras = f"[{','.join(xtr for xtr in sorted(ireq.extras))}]" if ireq.extras else ""
    return (
        f"{canonicalize_name(ireq.name)}{extras} @ "
        f"{ireq.link.url_without_fragment}"
        f"{fragment_string(ireq, omit_egg=True)}"
    )
```

but there I did not normalize the extra names, where you do convert to lower case.

I'd like to know:

- are we getting non-normalized extras names in the first place, or is the extra already normalized elsewhere?
- if not already normalized, we should probably run them through `canonicalize_name` rather than just lower casing, right?
- or is an extra sometimes not just a name, and so would be wrongly mangled this way?
AndydeCleyre: I forgot when I wrote that last comment that extra names are not at all package names, so we do NOT want to use canonicalize names on it. 

I guess I'd just like to see that any normalization is needed at this place. FWIW I just tested with #1329 an input file that specified an extra as `yAmL` and it came out as `yaml` in the txt. 🤷🏼 
FlorentJeannot: Hello @AndydeCleyre !

I remember your PR in #1329 but I was thinking, since mine is a small fix, that it could be merged quickly.
Though I wished that your PR was merged sooner than later. I like the way you handled the relative path and I think that's a nice feature for pip-tools!

Regarding the normalized names in the extras, I double-checked and `InstallRequirement` is parsing them in the first place. So I don't think we need to do anything else?
![Capture d’écran 2022-02-15 à 22 41 45](https://user-images.githubusercontent.com/12172017/154154490-963f4c27-4051-4249-aa15-ebe32269d132.png)

AndydeCleyre: Makes sense, and thanks again, as your feedback was central to getting that "right."

I won't close this, but also hope the big one gets merged soon. I will fix it up with your suggested change very soon I think. 
FlorentJeannot: Sounds good. Also if you have any good starter task to give, let me know and I'll happily contribute to the project.
atugushev: @FlorentJeannot According to this discussion https://github.com/jazzband/pip-tools/pull/1453#discussion_r671595549 I thought that we agreed to not include extras once requirements have been complied?
FlorentJeannot: @atugushev Sorry, it was not clear to me that my thinking was agreed or if it would be fixed later by @AndydeCleyre's PR (https://github.com/jazzband/pip-tools/pull/1329).
I thought some people would be confused and open issues to declare the bug, so I wanted to fix that now.

I guess we have two solutions: 
1. we merge this PR to restore the old behavior as a "fix" and later we make a PR which removes all extras in the compiled file
2. we decline this PR, and plan to make another one which removes all extras in the compiled file

Another question: Since @AndydeCleyre's PR contains the fix too, should we remove it there (so we don't bring more confusion by restoring the extras and then removing them)? Or should we make a PR now which removes them, so that @AndydeCleyre can merge this change and tweak his PR?
AndydeCleyre: I'm not convinced we should strip extras from the output, FWIW. 

The fact that pip freeze doesn't include the extras notation isn't necessarily a positive example, as the whole point of pip tools is that freeze is insufficient and too lossy with information for our needs. 
FlorentJeannot: @AndydeCleyre While I agree `pip-compile` should output more information than `pip freeze`, I'm not convinced it should contain the extras.
For example, package A has an `[extras]` which contains package `a` and `b`, and these are also extracted in the `.txt` file by `pip-compile`:

```
A[extras]==2.0
a==1.0
b==1.0
````

When I do `pip install` or `pip-sync`, then `pip` will then fetch `A==2.0` `A.a`, `A.b`, `a==1.0` and `b==1.0`. While this is not a big deal because it won't break the installation and install the correct version of `a` and `b`, it seems a bit redundant.

Also what if the dependency is compromised?

Let's say package `A` is updated and `[extras]` now contains `a`, `b` and `malware`. Then let's say I don't run `pip-compile -U` to update my `.txt` but I do `pip-sync` because I want to refresh the list of packages I've already pinned. `pip` will fetch `A==2.0` `A.a`, `A.b`, `A.malware`, `a==1.0` and `b==1.0`. So I think in this case we would install a package without realizing it, no?
AndydeCleyre: I honestly don't understand the example but want to point out in this discussion that some packages use extras to describe dependencies that are then detected during installation, affecting the installation procedure. 

If the relevant deps are just present in `reqs.txt`, installing with `pip install -r reqs.txt` will not guarantee installation order, which can break the `req[extra]` installation which needs the extras installed beforehand. Keeping them there, I think, guarantees the necessary order.
tal-rhino: It would be great to have this, or #1329, or any other solution soon.

I currently am having issues using pip-compile in a project requiring Celery with the "sqs" extra, because "Celery[sqs]" requires "kombu[sqs]" but pip-compile keeps only "kombu" for the sub-dependency :(
taleinat: It would be great to have this, or https://github.com/jazzband/pip-tools/pull/1329, or any other solution soon.

I currently am having issues using pip-compile in a project requiring Celery with the "sqs" extra, because "Celery[sqs]" requires "kombu[sqs]" but pip-compile keeps only "kombu" for the sub-dependency :(
ssbarnea: @atugushev What is your opinion on this one?

The way extras work (or not) makes me want to discourage their use, especially in result of pip-compile. Does everyone know that you can feed any typo to as an extra and pip will not complain? This proved to be a permanent source of problems as projects add/remove/rename extras over time.

I also have to confess that I am only using pre-commit to build constraints files, so I never want to see extras in them as the spec forbids them. That means that I should let others decide.
taleinat: FWIW, for my use case (properly installing `Celery[sqs]`), it would be fine if the output of `pip-compile` would include all of the required sub-dependencies without the extras.
atugushev: @ssbarnea I summed up my opinion [here](https://github.com/jazzband/pip-tools/issues/1613#issuecomment-1270338732).

Patch:
diff --git a/piptools/utils.py b/piptools/utils.py
index 4021753..fdfba40 100644
--- a/piptools/utils.py
+++ b/piptools/utils.py
@@ -155,7 +155,9 @@ def _build_direct_reference_best_efforts(ireq: InstallRequirement) -> str:
 
     # If we get here then we have a requirement that supports direct reference.
     # We need to remove the egg if it exists and keep the rest of the fragments.
-    direct_reference = f"{ireq.name.lower()} @ {ireq.link.url_without_fragment}"
+    lowered_ireq_name = canonicalize_name(ireq.name)
+    extras = f"[{','.join(sorted(ireq.extras))}]" if ireq.extras else ""
+    direct_reference = f"{lowered_ireq_name}{extras} @ {ireq.link.url_without_fragment}"
     fragments = []
 
     # Check if there is any fragment to add to the URI.


Test patch:
diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py
index ac75927..498b4a8 100644
--- a/tests/test_cli_compile.py
+++ b/tests/test_cli_compile.py
@@ -779,7 +779,10 @@ def test_direct_reference_with_extras(runner):
         )
     out = runner.invoke(cli, ["-n", "--rebuild", "--no-build-isolation"])
     assert out.exit_code == 0
-    assert "pip-tools @ git+https://github.com/jazzband/pip-tools@6.2.0" in out.stderr
+    assert (
+        "pip-tools[coverage,testing] @ git+https://github.com/jazzband/pip-tools@6.2.0"
+        in out.stderr
+    )
     assert "pytest==" in out.stderr
     assert "pytest-cov==" in out.stderr
 
@@ -2955,7 +2958,7 @@ def test_compile_recursive_extras(runner, tmp_path, current_resolver):
             os.fspath(tmp_path / "pyproject.toml"),
         ],
     )
-    expected = rf"""foo @ {tmp_path.as_uri()}
+    expected = rf"""foo[footest] @ {tmp_path.as_uri()}
 small-fake-a==0.2
 small-fake-b==0.3
 """
diff --git a/tests/test_utils.py b/tests/test_utils.py
index c549a0b..c89461f 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -79,6 +79,11 @@ def test_format_requirement(from_line):
             "example @ https://example.com/example.zip?egg=test",
             id="direct reference with egg in query",
         ),
+        pytest.param(
+            "example[b,c,a] @ https://example.com/example.zip",
+            "example[a,b,c] @ https://example.com/example.zip",
+            id="direct reference with optional dependency",
+        ),
         pytest.param(
             "file:./vendor/package.zip",
             "file:./vendor/package.zip",