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

Initial work on standard Python support for pyproject.toml files #5661

Merged
merged 10 commits into from
Oct 24, 2022

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 8, 2022

This adds initial support for pyproject.toml files.

Dependabot will be able to parse [project] dependencies and provide version and security updates. , but will not yet be able to parse [project.optional-dependencies]. EDIT: We also added support for updating dependencies under [project.optional-dependencies].

Since poetry can also uses pyproject.toml files, this also support parsing poetry dependencies. Using poetry's version constraint syntax was added in #5735

Here's an example PR generated with the `standard_python.toml` file

Pull Request Title: Bump ansys-templates from 0.3.0 to 0.3.1

Bumps ansys-templates from 0.3.0 to 0.3.1.

Commits

=> parsing dependency files
=> updating 1 dependencies: ansys-templates

=== ansys-templates (0.3.0)
 => checking for updates 1/1
🌍 https://pypi.org/simple/ansys-templates/
 => latest available version is 0.3.1
🌍 https://pypi.org/simple/ansys-templates/
 => latest allowed version is 0.3.1
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating ansys-templates from 0.3.0 to 0.3.1

    ± pyproject.toml
    ~~~
    12c12
    <    "ansys-templates==0.3.0",
    ---
    >    "ansys-templates==0.3.1",
    ~~~
🌍 https://pypi.org/pypi/ansys-templates/json
🌍 https://github.com/pyansys/pyansys-template.git/info/refs
🌍 https://github.com/pyansys/pyansys-template.git/info/refs

@deivid-rodriguez deivid-rodriguez self-assigned this Sep 8, 2022
@Nishnha Nishnha force-pushed the deivid-rodriguez/standard-python branch from 954e0d6 to 874e35c Compare September 23, 2022 01:12
@Nishnha Nishnha marked this pull request as ready for review September 23, 2022 01:12
@Nishnha Nishnha requested a review from a team as a code owner September 23, 2022 01:12
@Nishnha
Copy link
Member

Nishnha commented Sep 23, 2022

I'd be curious if this can handle something as complex as docker-compose's requirements
https://github.com/docker/compose/blob/789bfb0e8b2e61f15f423d371508b698c64b057f/setup.py#L28-L61

Copy link
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

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

I know very little about Poetry or pep621 but I gave this a first pass. Nice job keeping these changes to a minimum!

Is there any test coverage for doing a standard python update? I see from the PR description that it can perform updates but I'm not seeing an added test case for the updater. It'd be great to have one (or more) if it's not there!

Also, is there anything else missing besides the optional-dependencies? Would we want to feature flag this till we have more complete or tested support?

python/lib/dependabot/python/file_fetcher.rb Outdated Show resolved Hide resolved
python/spec/dependabot/python/file_parser_spec.rb Outdated Show resolved Hide resolved
@deivid-rodriguez
Copy link
Contributor Author

Thanks for having a look @mctofu!

Is there any test coverage for doing a standard python update? I see from the PR description that it can perform updates but I'm not seeing an added test case for the updater. It'd be great to have one (or more) if it's not there!

That's the one task I had pending before considering this PR finished, I will add those soon :)

Also, is there anything else missing besides the optional-dependencies? Would we want to feature flag this till we have more complete or tested support?

We could also potentially support updating build dependencies. See this comment for useful links. But that sounds much trickier and I'm not sure how many people would find it useful, so I would not add support for now. Regarding feature flagging this, I'm not sure. It seems a simple update but it's true that Dependabot will start considering many files it didn't consider before so maybe it makes sense, yeah.

@deivid-rodriguez
Copy link
Contributor Author

I'm finding a few more things missing, so I'll recap those here. I believe all of them are suitable for follow up work, and not necessarily need to block this PR.

@davidism
Copy link

davidism commented Sep 29, 2022

It seems like the focus is on pyproject.toml itself, but bumping versions there might not make sense in many cases.

For example, pip-tools separates requirements.in, the template listing of direct dependencies and broad constraints, from requirements.txt that is the full dependency tree with exact versions. Dependabot updates the txt pins, not the in template.

For pyproject.toml based tools, I'd expect the same thing: the pyproject file is the template with constraints, and tools generate a lock file with specific versions that I'd want bumped. For example, PDM generates pdm.lock, another TOML file. Unfortunately lock files are not standardized, but it sounds like you're already considering multiple behaviors for multiple tools.

@deivid-rodriguez
Copy link
Contributor Author

@davidism The behavior you're suggesting is implemented by the lockfile-only versioning strategy, but of course that only makes sense if there's a lock file. In the case of Standard Python, lockfiles are not defined as you point out, so we only focus on the manifest and not on the lock file. Hope this makes sense!

I'm now satisfied with this PR, by the way! 😄

@davidism
Copy link

davidism commented Oct 3, 2022

I'm worried that only supporting pins in pyproject.toml, rather than waiting for lockfiles, will encourage users to put pins there. That's probably fine for applications, but would be disastrous if libraries started pinning their dependencies like that. Please reconsider supporting some common lock file formats, since you already have custom support for Poetry.

@davidism davidism mentioned this pull request Oct 3, 2022
@deivid-rodriguez
Copy link
Contributor Author

I'm worried that only supporting pins in pyproject.toml, rather than waiting for lockfiles, will encourage users to put pins there. That's probably fine for applications, but would be disastrous if libraries started pinning their dependencies like that.

Dependabot does not encourage any way of handling dependencies, it respects what users are already doing, and just tries to help them keep things up to date. So it will only use pins in pryproject.toml files if they are already using pins. If they are using fully unrestricted dependencies, then dependabot will not propose any updates.

Please reconsider supporting some common lock file formats, since you already have custom support for Poetry.

This is just a first iteration, we will be happy to add support for lock file formats using standard Python in the future.

@davidism
Copy link

davidism commented Oct 3, 2022

I'm worried that users seeing that Dependabot supports pins in the file will encourage users to put pins there, not that Dependabot will put them there itself. Basically, supporting this will mean that more people use it, when it's probably not the best practice to encourage.

@deivid-rodriguez
Copy link
Contributor Author

How will users "see" that we support pins, if they are not already using pins? I'm not following your logic of how Dependabot will encourage a bad thing here.

@davidism
Copy link

davidism commented Oct 3, 2022

Presumably by seeing an announcement that Dependabot supports pins in pyproject.toml, or reading the Dependabot documentation about what pins it supports.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Oct 3, 2022

Presumably by seeing an announcement that Dependabot supports pins in pyproject.toml, or reading the Dependabot documentation about what pins it supports.

I don't think we should explicitly announce or document that we support pins in pyproject.toml. We should announce that we support projects using pyproject.toml files, in whatever way people are managing their dependencies. We should of course try to not encourage bad practices in our blog posts or documentation.

Also, by "pins", do you mean "full pins" (like == 0.0.3) or any way of restricting dependencies? Because we not only support full pins, but also other perfectly valid ways of restricting dependencies. Just in case there's some misunderstanding there.

In any case, I'm pretty sure that library authors fully pinning their dependencies are going to find out how that's bad the hard way, regardless of Dependabot 😅.

@ofek
Copy link

ofek commented Oct 24, 2022

Everyone is excited for this 😄

deivid-rodriguez and others added 10 commits October 24, 2022 18:02
It's no longer used since 2115484.
It's unused since it was first introduced at
65ee6f8, since it's duplicated in
`PoetryFilesParser`.
To make room for standard python specs and clarify what fixture
manifests test.
Reuse the same logic used for detecting poetry libraries. Also for
consistency, since standard Python does not have a caret operator, change existing
poetry update checker specs to use the tilde operator instead, which
is also implemented in standard python.
@deivid-rodriguez deivid-rodriguez changed the title initial work on standard Python support Initial work on standard Python support Oct 24, 2022
@deivid-rodriguez
Copy link
Contributor Author

Alright let's do this! 🎢

@deivid-rodriguez deivid-rodriguez merged commit 417a6c3 into main Oct 24, 2022
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/standard-python branch October 24, 2022 19:41
@pavera pavera mentioned this pull request Oct 31, 2022
@jeffwidman jeffwidman changed the title Initial work on standard Python support Initial work on standard Python support for pyproject.toml files Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants