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 packaging.LegacyVersion use with mozilla_version. Fixes #635 #638

Merged
merged 4 commits into from Apr 17, 2023

Conversation

jfx2006
Copy link
Contributor

@jfx2006 jfx2006 commented Feb 28, 2023

As packaging.LegacyVersion is gone, and packaging.Version produces too many errors, use the various version classes from mozilla_version.gecko when looking for the latest version of a supported application.

…rsion. Fixes mozilla#635

As packaging.LegacyVersion is gone, and packaging.Version produces
too many errors, use the various version classes from mozilla_version.gecko
when looking for the latest version of a supported application.
@jfx2006 jfx2006 changed the title Replace packaging.LegacyVersion use with mozilla_version. . Fixes #635 Replace packaging.LegacyVersion use with mozilla_version. Fixes #635 Feb 28, 2023
@jfx2006
Copy link
Contributor Author

jfx2006 commented Feb 28, 2023

A possible fix for #635 that doesn't require maintaining version parsing code in this project. mozilla_version is used by several Mozilla projects and is maintained by Releng.

I ran tox locally before submitting.

py37: SKIP (0.00 seconds)
py38: OK (133.76=setup[13.18]+cmd[120.42,0.16] seconds)
py39: OK (132.65=setup[12.18]+cmd[120.31,0.16] seconds)
pylama: OK (0.92=setup[0.60]+cmd[0.32] seconds)
congratulations :) (267.65 seconds)

mozilla_version itself is roughly the same size as packaging. It does unfortunately pull in futures as a dependency, which is kind of hefty at 2.7M.

Alternatively, please consider updating requirements.txt to only allow packaging >= 19.0, < 22.0 as mozdownload currently pulls in version 23.0 and doesn't work with -v latest as a result.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Oh, I thought that I had fixed the pinning with #636. But as I notice now I only did that for the tests. :(

Thanks a lot for this suggestion! I didn't know yet that we even have such a package. That is great and I'm all for using it. Also because I'm not a fan of using protected like methods from a foreign library.

I just triggered the tests and those show a failure for Thunderbird version checks. Maybe you could have a look at it? Thanks!

@jfx2006
Copy link
Contributor Author

jfx2006 commented Feb 28, 2023

That's odd, since I can't get it to fail locally after commenting out @pytest.mark.ci_only on those tests. I will look at it more tomorrow.

@whimboo
Copy link
Contributor

whimboo commented Mar 1, 2023

Sounds fine. Let me know if you need any help. At least it happens for the test with version=latest only. So maybe we collect some folder / version which causes this problem.

@jfx2006
Copy link
Contributor Author

jfx2006 commented Mar 1, 2023

Okay, I figured it out.

mozilla_version expects that the old-fourth number of a release is strictly positive (> 0), and there was apparently a Thunderbird 2.0.0.0 which violates that rule. There was no Firefox 2.0.0.0, just a 2.0.0.1, so that explains why the error doesn't trigger for Firefox.

There is code in mozilla_version meant to deal with exceptions, but it doesn't trigger until the end of the parse classmethod, so there's no easy fix there. I will file a bug in that project and see what I can do.

@whimboo
Copy link
Contributor

whimboo commented Mar 3, 2023

There is code in mozilla_version meant to deal with exceptions, but it doesn't trigger until the end of the parse classmethod, so there's no easy fix there. I will file a bug in that project and see what I can do.

Thanks a lot. Please reference the issue here as well. I would be interested. Thanks!

@whimboo
Copy link
Contributor

whimboo commented Mar 9, 2023

@jfx2006 do you have an update? If not I wonder if you could create a small PR which restricts the usage of packaging to the last supported version. I would happily review such a patch to get the tool working again.

@whimboo
Copy link
Contributor

whimboo commented Mar 9, 2023

If you want to please also see #637 (comment) for an extra entry for Dependabot so that we do no longer get upgrade PRs.

@whimboo
Copy link
Contributor

whimboo commented Apr 5, 2023

@jfx2006 I wonder if you would have the time to work on this issue. In the meantime I pushed #640 to fix the problem with the packaging version that gets pulled in when installing mozdownload. As such a rebase of this PR would be necessary.

Thanks!

@jfx2006
Copy link
Contributor Author

jfx2006 commented Apr 6, 2023

This is kind of ugly. :(

It occurred to me, product-details might be a better place to check for "latest" versions. Candidate releases will still need to scrape, but the latest released versions will be in either "firefox_versions.json" or "thunderbird_versions.json". Some other day...

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Thank you for the update! Just two small things to discuss/fix. Then it looks fine to me.

Also note that the mozilla-package could easily stop using the future package. I filed mozilla-releng/mozilla-version#102 to get rid of it. As such I'm fine with this approach.

mozdownload/scraper.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Looks fine to me now. Thanks a lot for the contribution!

@whimboo whimboo merged commit 72abd7c into mozilla:master Apr 17, 2023
6 checks passed
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.

None yet

2 participants