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

Replace deprecated distutils.version.StrictVersion. #433

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

znewman01
Copy link
Contributor

distutils is deprecated as of Python 3.10. We were only using it for StrictVersion, which parses a version string and lets you order the result.

The official recommendation from the packaging team is to replace StrictVersion with a dependency on the packaging package, but:

  1. The Python version parser was a little different from GPG versioning anyway because it's made for Python versions (it had support for "prerelease tags").
  2. I wanted to avoid adding dependencies if possible.

This PR adds a new Version class in securesystemslib.gpg.util and uses that instead.

It's a breaking change to gpg.util.get_version(), but AFAICT that wasn't ever intended for public consumption.

Signed-off-by: Zachary Newman [email protected]

Fixes: N/A

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

distutils is deprecated as of Python 3.10. We were only using it for
`StrictVersion`, which parses a version string and lets you order the
result.

The [official recommendation] from the packaging team is to replace
`StrictVersion` with a dependency on the `packaging` package, but:

1. The Python version parser was a little different from GPG versioning
   anyway because it's made for Python versions (it had support for
   "prerelease tags").
2. I wanted to avoid adding dependencies if possible.

[official recommendation]: pypa/packaging#520

This PR adds a new `Version` class in `securesystemslib.gpg.util` and
uses that instead.

Signed-off-by: Zachary Newman <[email protected]>
@jku
Copy link
Collaborator

jku commented Oct 3, 2022

This may be off topic for your PR but since you're touching this you may have an opinion: I wonder how much work would it be to use an actual API like https://pypi.org/project/python-gnupg/ or https://pypi.org/project/gpg/ for the ~2 gpg commands we need? Spawning new processes via a command line makes me feel dirty (and is a great source of testing trouble).

@joshuagl
Copy link
Collaborator

joshuagl commented Oct 3, 2022

Some related discussion in #428
tl;dr – we don't want to run gpg commands, but we don't want to add dependencies either. Discussion in #428 strives to find a middle ground.

@znewman01
Copy link
Contributor Author

Spawning new processes via a command line makes me feel dirty (and is a great source of testing trouble).

I'm generally sympathetic to that point of view. While I do think minimizing dependencies is nice, to my mind, there's not much difference between "depends on GPG" and "depends on GPG+Python bindings to GPG."

That said, my most immediate goal is to fix #428 and reduce flakiness in CI for go-tuf; I think the very minor change in #429 would fix it but we need to couple that with some extra testing (#434).

Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, especially if we consider the whole "library spawns other binaries and parses output" as a hack that securesystemslib should probably get rid of.

Left a comment but even that does not seem critical (I don't think it'll break anything in this decade)

Comment on lines +836 to +837
'1.3',
'1.33.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are both valid for StrictVersion.

Obviously we don't need to re-implement StrictVersion here (we know this is a hack, it just has to work well enough for this specific case) but at least the latter one seems strange to me -- is this just because gpg historically hasn't used multidigit minor versions?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. Unfortunately, I can't remember if it was just an assumption based on the existing version progression (e.g. last 1.x was 1.9), or if there was a stronger indication somewhere. Happy to change, although not necessarily in this PR, which just uses the regex that was already there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh you're right: no new bugs then :)

@lukpueh
Copy link
Member

lukpueh commented Oct 4, 2022

I wonder how much work would it be to use an actual API like https://pypi.org/project/python-gnupg/ or https://pypi.org/project/gpg/

Too much work for the benefit. See #247 and https://wiki.python.org/moin/GnuPrivacyGuard

Tl;DR: Both packages you suggest also just spawn a process to run the gpg command, the former via subprocess, the latter via GPGME C binding. So far we haven't found a good alternatives to our "hack".

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Great work. Looks more robust, than it was before. Regarding backwards-compatibility, I confirm that gpg.util.get_version() was not intended as public API.

@lukpueh lukpueh merged commit 320f89d into secure-systems-lab:master Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants