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

extras are not validated against dependencies #7226

Closed
3 tasks done
Panaetius opened this issue Dec 20, 2022 · 8 comments · Fixed by python-poetry/poetry-core#542
Closed
3 tasks done

extras are not validated against dependencies #7226

Panaetius opened this issue Dec 20, 2022 · 8 comments · Fixed by python-poetry/poetry-core#542
Labels
area/pyproject Metadata/pyproject.toml-related kind/feature Feature requests/implementations

Comments

@Panaetius
Copy link

  • Poetry version: 1.3.1
  • Python version: 3.9.15
  • I am on the latest stable Poetry version, installed using a recommended method.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have consulted the FAQ and blog for any relevant entries or release notes.

Issue

To give an example, with a pyproject.toml like

[tool.poetry]
name = "test"

[tool.poetry.dependencies]
numpy = { version = "^1.20.0", optional = true }

[tool.poetry.extras]
my_extra = ["numpy "]

poetry lock, poetry check and poetry install -E my_extra all run without any error or anything. But they wouldn't install the numpy dependency. This is because there's an extra space in my_extra = ["numpy "] that means the extra isn't matched to the dependency. This would happen with any misspelling in the my_extra list. So poetry just silently ignores entries in extras that can't be matched to a dependency.

The poetry.lock file would just have a sections like

[extras]
my_extra = []

in this case.

It'd be nice to at least get a warning in poetry lock or poetry check when an extra item doesn't match a dependency. These types of errors can be hard to spot and can take a while to figure out.

@Panaetius Panaetius added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Dec 20, 2022
@neersighted
Copy link
Member

This is by design, the optional = true field is not directly tied to extras. Instead it merely marks a dependency as optional in the solver, which means it will be excluded unless something else (like an extra) makes it part of the solution. You can think of it not unlike pip's constraints.txt. Whether or not that is good design is another issue entirely; I don't think adjustments will be made here unless we redesign how extras are declared.

@neersighted neersighted added kind/feature Feature requests/implementations area/pyproject Metadata/pyproject.toml-related and removed kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Dec 21, 2022
@neersighted
Copy link
Member

I'm going to close this for now as it's currently how this is designed; proposals for a better way to define extras are welcome, which could include the validations here (if we had something more specific than optional which is not specific to extras).

@neersighted neersighted closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2022
@Panaetius
Copy link
Author

I think the way optional works is fine.

What is wrong in my opinion though is that tool.poetry.extras can refer to non-existing things. The solver should at least validate that the extras point to something valid. And it's not even just that the extra field doesn't have to refer to an actual dependency, there's no input validation at all.

I mean, take this for example:

[tool]

[tool.poetry]
name = "test"
version = "1.0.0"
description = ""
authors = ["Test <test@example.com>"]

[tool.poetry.extras]
my_extra = ["<a href='something.html'>;$öä+*%&</a>"]

and use it

$ poetry check
All set!
$ poetry lock
Updating dependencies
Resolving dependencies... (0.1s)

Writing lock file
$ poetry install -E my_extra
Installing dependencies from lock file

I don't think this is related to optional, it's just that extras themselves aren't validated. The docs even say "The dependencies specified for each extra must already be defined as project dependencies." but this "must" isn't enforced.

Specifically:
Validate this with the ^[a-zA-Z-_.0-9]+$ regex
Change this to something like

            # Checking for dependency
            for req in requirements:
                req = Dependency(req, "*")
                for dep in package.requires:
                    if dep.name == req.name:
                        dep.in_extras.append(extra_name)
                        package.extras[extra_name].append(dep)
                        break
                else:
                    print(f"Warning: Extra {req.name} not found in dependencies.")

Not sure if there are cases where a dependency is specified several times, then the above would not work and instead you need some found = True style logic. I'm not really familiar with the internals of poetry.

@Panaetius
Copy link
Author

python-poetry/poetry-core#542 Here would be a PR for it that fixes it, at least for poetry check

@radoering
Copy link
Member

radoering commented Dec 31, 2022

It seems that this issue was closed due to a misunderstanding. If I don't miss anything, it doesn't make sense to reference a non-existent dependency in an extra. And since we define extras as arrays of dependencies, it makes sense to apply the same regex as for dependency names for validation.

Not sure if there are cases where a dependency is specified several times, then the above would not work and instead you need some found = True style logic. I'm not really familiar with the internals of poetry.

There are multiple-constraints dependencies but I'm afraid these don't work well with extras anyway.

@radoering radoering reopened this Dec 31, 2022
@cutwater
Copy link

cutwater commented Jan 5, 2023

This is by design, the optional = true field is not directly tied to extras.

@neersighted Has the design changed? Earlier same year @finswimmer, a member of poetry org, made directly the opposite statement in response to the issue #2357:

Any optional dependency must be assigned to an extra

If the design has changed, can you please give any reference to the document that describes it. Otherwise which statement should be considered as the source of truth?

@neersighted
Copy link
Member

Otherwise which statement should be considered as the source of truth?

They're both correct, I'm talking about the code as it exists today and the current architecture of how we define extras; @finswimmer is talking about the fact that while optional dependencies can be used to influence the solver, we won't capture them in the package's requirements unless they are assigned to an extra.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/pyproject Metadata/pyproject.toml-related kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants