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

py-metomi-isodatetime: fix url parsing #41415

Merged
merged 3 commits into from Jan 10, 2024

Conversation

adamjstewart
Copy link
Member

Closes #41414

Could have modified our regexes to better support this version identifier but this is literally the only case we have in Spack as far as I know so will wait until this is more common.

@climbfuji
Copy link
Contributor

Thanks @adamjstewart. I tried your PR (cherry-picked your commit), but still getting:

[cylctest-20231204] heinzell@JCSDA-L-18146:~/work/spack-stack/spack-stack-cylc8 [brew-arch64]> spack checksum py-metomi-isodatetime@3.0.0
==> Error: Couldn't detect version in: https://files.pythonhosted.org/packages/source/m/metomi-isodatetime/metomi-isodatetime-1!3.0.0.tar.gz
[cylctest-20231204] heinzell@JCSDA-L-18146:~/work/spack-stack/spack-stack-cylc8 [brew-arch64]> spack checksum py-metomi-isodatetime@3.1.0
==> Error: Couldn't detect version in: https://files.pythonhosted.org/packages/source/m/metomi-isodatetime/metomi-isodatetime-1!3.0.0.tar.gz

Am I doing something wrong?

tldahlgren
tldahlgren previously approved these changes Dec 4, 2023
Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Confirmed the package's version sha256 are still valid with the change. Just a simple question.

@tldahlgren tldahlgren self-assigned this Dec 4, 2023
@climbfuji
Copy link
Contributor

@tldahlgren This is unfortunately not working for me, but it's likely that it is a user error.

@tldahlgren
Copy link
Contributor

@tldahlgren This is unfortunately not working for me, but it's likely that it is a user error.

Which part? The PR or the use of an f-string?

@climbfuji
Copy link
Contributor

@tldahlgren This is unfortunately not working for me, but it's likely that it is a user error.

Which part? The PR or the use of an f-string?

The PR itself (sorry for not being clear)

@adamjstewart
Copy link
Member Author

I didn't fix:

> spack checksum py-metomi-isodatetime

I did fix:

> spack checksum py-metomi-isodatetime 3.1.0

The former is a completely different set of Spack code. I can fix it if you want, but will need to find time to adjust the regexes to add support for this.

@adamjstewart
Copy link
Member Author

Ah, apparently my solution only works for versions that have already been checksummed. Will need to adjust the regex instead.

@adamjstewart adamjstewart marked this pull request as draft December 5, 2023 08:37
@adamjstewart
Copy link
Member Author

We need to decide whether or not we care about the epoch number in https://peps.python.org/pep-0440/#public-version-identifiers. Like, should the version be called 1!3.0.0 or 3.0.0? The epoch is designed for when a package changes from something like 2022.11.12 to 1.2.3. Normally, the former would sort as newer than the latter, but the epoch allows you to correctly sort these.

@becker33
Copy link
Member

A few core Spack developers discussed this today. Our proposal is to model the epoch in the same manner that Python does (and Fedora as well). We will propose a PR with the N!A.B.C version syntax supported. Versions without an epoch will have an implicit 0 epoch. Infinite versions (develop, main, etc) will have an implicit infinite epoch so they always remain "greater than" every other version.

@climbfuji
Copy link
Contributor

A few core Spack developers discussed this today. Our proposal is to model the epoch in the same manner that Python does (and Fedora as well). We will propose a PR with the N!A.B.C version syntax supported. Versions without an epoch will have an implicit 0 epoch. Infinite versions (develop, main, etc) will have an implicit infinite epoch so they always remain "greater than" every other version.

That sounds great, thanks for following up @becker33 !

@adamjstewart
Copy link
Member Author

To me, this seems like it should be much lower priority (only affects 1 package) than supporting a/b/rc/dev as being less than stable (affects most packages).

@becker33
Copy link
Member

@adamjstewart while I agree about impact, I think it's much easier to support than the more important issue around pre-release versions.

@LydDeb
Copy link
Contributor

LydDeb commented Dec 14, 2023

For information purposes, after scanning all 500644 packages present on Pypi, I identified 40 which appear to be concerned by the Epoch segment (the character "!" is present in an URL of a release).

@adamjstewart
Copy link
Member Author

That's impressive work. So only 0.008% of packages.

@LydDeb
Copy link
Contributor

LydDeb commented Dec 15, 2023

It was a bit long. The python modules xmlrpc.client and requests were very useful for listing all the packages and retrieving useful information.
What is the final decision regarding support for Epoch versioning syntax?

@adamjstewart adamjstewart marked this pull request as ready for review January 6, 2024 16:28
@adamjstewart
Copy link
Member Author

Should we merge this as is? It doesn't fix:

> spack checksum py-metomi-isodatetime

but it does fix:

> spack fetch py-metomi-isodate

The latter is much more important.

@LydDeb
Copy link
Contributor

LydDeb commented Jan 9, 2024

I think we can keep this modification which allows the installation of the package. We could possibly add a comment indicating that the checksum does not work.

Copy link
Contributor

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I concur, a note that the checksum feature doesn't work for this package would be nice.

@tldahlgren tldahlgren merged commit fb320ec into spack:develop Jan 10, 2024
13 checks passed
@adamjstewart adamjstewart deleted the packages/py-metomi-isodatetime branch January 10, 2024 21:49
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jan 25, 2024
* py-metomi-isodatetime: fix url parsing

* One-liner

* Add note that checksum doesn't work
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jan 31, 2024
* py-metomi-isodatetime: fix url parsing

* One-liner

* Add note that checksum doesn't work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

checksum mismatch for py-metomi-isodatetime@3.1.0
5 participants