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 parsing error on python_requires >=3.0.* by simplifying to >=3 #3

Merged
merged 1 commit into from Jul 21, 2023

Conversation

YSaxon
Copy link
Contributor

@YSaxon YSaxon commented Apr 17, 2023

Under certain circumstances the existing python_requires string fails out the parser and causes the pip install to fail. Simplifying it should fix that.
Screenshot 2023-04-17 at 5 01 24 PM

@shailshouryya
Copy link
Owner

Hi YSaxon, thanks so much for making this fix! I just ran into something similar when I tried uploading this package to PyPI just now, which I worked around with commit 21bd027 - however, I think your approach is better since it maintains the >= (my update changes the comparison to >), and I also didn't quite understand what caused the python_requires check to start failing.

Can you give a quick explanation of what the reason for this failure is/why this change fixes it?

Also, thank you for following the same style I've been using for the git commit messages - I've been meaning to add a CONTRIBUTING.md for this, but you were very consistent even without it!

@shailshouryya
Copy link
Owner

Also, looks like I rebased my commits and created conflicts with your original fix 😅 - would you prefer to rebase your change on top of my rebase (probably something like git pull origin upstream --rebase) or would you prefer me to fix the conflicts up here and merge in your change?

@shailshouryya shailshouryya deleted the branch shailshouryya:main June 14, 2023 19:28
@shailshouryya
Copy link
Owner

I didn't close this pull request, but I was playing around with the branches earlier and it looks like GitHub automatically closes any pull requests that are associated with a particular branch that gets modified, which is interesting - but not ideal here 😂

Your changes look good to me though!

@shailshouryya shailshouryya reopened this Jun 14, 2023
@shailshouryya
Copy link
Owner

shailshouryya commented Jun 16, 2023

Did some digging and found the likely culprit for what caused this problem:

It looks like this is the expected behavior as defined in PEP 440 under the Inclusive ordered comparison section:

An inclusive ordered comparison clause includes a comparison operator and a version identifier, and will match any version where the comparison is correct based on the relative position of the candidate version and the specified version given the consistent ordering defined by the standard Version scheme.

Following the link to the Version scheme section and looking at the specification under the Public version identifiers section:

The canonical public version identifiers MUST comply with the following scheme:
[N!]N(.N)*[{a|b|rc}N][.postN][.devN]
Public version identifiers MUST NOT include leading or trailing whitespace.

Public version identifiers MUST be unique within a given distribution.
...

The last line included above seems to be the "loose implementation" of the version modifier that the issues and pull requests I linked to at the very top were discussing ("Did some digging and found the likely culprit").

Once that "loose implementation" was fixed, any package that didn't follow the PEP 440 specification for version identifiers broke. In this package, the break occurred because of the >=3.0.* comparison, which IS NOT unique within a given distribution, as opposed to >=3 (what your fix here does), which IS unique within a given distribution.

To clarify: it looks like the problem we see in this issue is because of implementation fixes in the packaging tools to more closely follow PEP 440, specifically "Public version identifiers MUST be unique within a given distribution." Any package that relied on the previous implementation that loosely verified the PEP 440 specification but did not strictly follow PEP 440 broke after the implementation of the packaging tool(s) were fixed to more closely follow PEP 440. More explicitly (from this comment on the How to pin a package to a specific major version or lower discussion):

Christopher already made the response I was going to make: for PEP 440 as written, using wildcards outside of “==” and “!=” comparisons isn’t valid.

Allowing them for “>=” and “<=” would be reasonable, but it would involve a PEP to fix the spec. (It wasn’t a conscious choice to exclude them, we just didn’t notice at the time that the inclusive ordered comparison operators weren’t formally defined as combining an exclusive ordered comparison with a version match, so the tools have been written to ignore the wildcard instead of paying attention to it)

Making a coherent definition wouldn’t be too hard: just ignore the wildcard for the exclusive ordered comparison part and keep it for the version matching part.

Here are some other posts that aren't directly relevant, but might be interesting tangents for anyone interested in packaging problems:

@shailshouryya
Copy link
Owner

Oof, the merge/rebase diff is a bit ugly here 😅 - here might be a simpler way to resolve the conflicts:

  1. Go to your fork of the repo on your terminal/command prompt: cd /path/to/save-thread-result
  2. Switch to the main branch of the fork: git checkout main
  3. Remove the conflicting commit because of my rebase: git reset --hard HEAD~1
  4. Sync this removed commit to your remote repository: git push --force origin main (you need the --force option since this is removing a commit)
  5. Go to your fork on the GitHub UI and click the Sync fork button under the |Go to file| |Add file| |<> Code| buttons. You should get a dropdown that says something like the following with a green Update branch button at the bottom:

This branch is out-of-date
Update branch to keep this branch up-to-date by syncing N commits from the upstream repository.
Learn more

  1. Click the Update branch button.
  2. Pull the updated, synchronized git commits to your local repository: git pull origin main
  3. Apply your fix here: git cherry-pick 41ebc90af1b56c4c5257cfd071b926900e3b5ec5
    a. This is probably where you'll get the merge conflict - this one should be much simpler though, and only have a conflict in the setup.py file - resolve the conflict here
    b. Commit the resolved conflicts: git add python/setup.py && git commit
    c. Update your patch-1 branch: git branch -D patch-1 && git checkout -b patch-1
  4. Push the resolved fixes to the branch: git push --force origin patch-1 (you'll probably need the --force flag again because the git history has diverged)

I think this should be enough to resolve the conflicting files issue - let me know if you run into any problems though!

@YSaxon
Copy link
Contributor Author

YSaxon commented Jul 20, 2023

Thanks for the detailed steps, and the followup. Let me know if this works

@shailshouryya
Copy link
Owner

This, works, thank you! 🎉

@shailshouryya shailshouryya merged commit d3e02a7 into shailshouryya:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants