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

fix #8614 source constriants failed with extras #8802

Conversation

lucemia
Copy link
Contributor

@lucemia lucemia commented Dec 18, 2023

Pull Request Check List

Resolves: #8614

  • Added tests for changed code.
  • Updated documentation for changed code.

@lucemia lucemia force-pushed the fix-8514-source-constriants-failed-with-extras branch from e374137 to 2c8864d Compare December 18, 2023 07:18
@lucemia lucemia changed the title fix #8514 source constriants failed with extras fix #8614 source constriants failed with extras Dec 18, 2023
@lucemia lucemia force-pushed the fix-8514-source-constriants-failed-with-extras branch from 2c8864d to ebf8852 Compare December 18, 2023 07:40
@lucemia lucemia marked this pull request as ready for review December 18, 2023 07:44
@latk
Copy link

latk commented Dec 22, 2023

Thank you so much for trying to fix this! It seems that this proposed code is evaluated quite early in the Poetry lifecycle, so that you're working with the raw TOML structure:

        for group in [*local_config.get("group", {}).values(), local_config]:
            for name, dependency in group.get("dependencies", {}).items():
                if isinstance(dependency, dict) and "source" in dependency:
                    dependency_source_cache[name] = dependency["source"]

This is good, because it runs before packages can be looked up.

But I'm not sure whether it supports all documented dependency specification forms, in particular multiple constraint dependencies where the entry might be a list, not a dict.

The docs give this example:

[tool.poetry.dependencies]
foo = [
    {version = "<=1.9", python = ">=3.6,<3.8"},
    {version = "^2.0", python = ">=3.8"}
]

In #8614, I noted that each candidate's source might differ. For example:

some-dep = [
  { ..., source = "repo-a", python = ">=3.6,<3.8" },
  { ..., source = "repo-b", python = ">=3.8" },
]

I'm not sure whether Poetry actually supports this.

  • If yes, I don't think the name–source mapping suggested in this PR would work. We'd need a better "key" than the dependency name.
  • If it isn't supported, this mapping should at least check that there's no contradictory source information. For security-relevant features like source, I think a hard error is preferable to silently proceeding with an unclear choice.

@lucemia
Copy link
Contributor Author

lucemia commented Dec 22, 2023

Thank you for your feedback. Yes, I did forget about handling multiple constraint dependencies, I will add support for them.

As you pointed out, the current pull request doesn't completely resolve the issue if we don't ensure the consistency of source information. I'm also addressing the related problem of "Package's source inconsistency" described in #8704. However, I still don't have a clear idea of how to develop it correctly. Did poetry already handle this issue somewhere? I don't have an idea yet.

The primary goal of the pull request is to provide a partial solution that is at least on par with the current situation. This is because the mapping comes into play only when the repository_name is not provided.

@radoering
Copy link
Member

I'm not sure if this is the right path, but since I have no other solution in mind, I'll give some feedback:

I'm not sure whether Poetry actually supports this.

It does. name + markers (including python and platform) should be sufficient. (IIRC, platform and python is integrated in markers internally.) However, markers can't be used as a simple key because the lookup might not be with the original marker but a subset of it. Further, we don't pass markers to the RepositoryPool at the moment, so this might not be the best place for the lookup...

working with the raw TOML structure

It's probably better to use the parsed dependencies in package.requires because python and platform have already been merged with markers and multiple-constraints dependencies should be parsed at that point.

@radoering
Copy link
Member

After thinking about it again, I believe that this issue should be addressed in the solver instead of the repository pool. The repository pool is not aware of markers (and probably should not be) so that it is not possible to handle multiple constraints with different sources.

I gave it a try in #8835 and I am not aware of any drawbacks yet.

Copy link

github-actions bot commented Mar 3, 2024

This pull request 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 Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package source constraints are silently ignored when extras and transitive dependencies are involved
3 participants