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

Support Python3 #41

Merged
merged 53 commits into from
Jan 14, 2022
Merged

Support Python3 #41

merged 53 commits into from
Jan 14, 2022

Conversation

exarkun
Copy link
Contributor

@exarkun exarkun commented Jan 12, 2022

... and also stop testing Python 2 support

I also ended up mixing a lot of CircleCI configuration changes in here to do a better job of building wheels. It's possible this wasn't strictly necessary but it doesn't end up making the PR diff that big so I guess I'm planning to leave it here (the history is, of course, the usual mess of "does it work now?" that you see with most cloud service configuration reconfigurations...).

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link

@tomprince tomprince left a comment

Choose a reason for hiding this comment

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

This looks fine with a couple of changes:

  • using build instead of pip for creating wheels
  • adding the comments around the arm circleci image and renaming it


- run:
name: "Build Wheel"
command: |
python setup.py bdist_wheel
<< parameters.python >> -m pip wheel .

Choose a reason for hiding this comment

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

I'd suggest using python -m build (which uses build) or at least passing --no-deps. The current command builds (or fetches) wheels for all dependencies of the specified packages as well.

It looks like build puts the files in dist/, instead of the current directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. It looks like python -m build has a problem with the way the package is set up:

* Building wheel from sdist                                                                                                                                                       
* Creating venv isolated environment...
* Installing packages in isolated environment... (setuptools >= 40.8.0, wheel)        
* Getting dependencies for wheel... 
* Installing packages in isolated environment... (milksnake, setuptools_scm, wheel)   
* Building wheel...                 
running bdist_wheel                                                                      
running build                                                                            
running build_py                                                                         
creating build/lib                                                                       
creating build/lib/challenge_bypass_ristretto                                        
copying challenge_bypass_ristretto/__init__.py -> build/lib/challenge_bypass_ristretto
creating build/lib/challenge_bypass_ristretto/tests
copying challenge_bypass_ristretto/tests/__init__.py -> build/lib/challenge_bypass_ristretto/tests
copying challenge_bypass_ristretto/tests/test_privacypass.py -> build/lib/challenge_bypass_ristretto/tests
error: [Errno 2] No such file or directory: './challenge-bypass-ristretto-ffi'

ERROR Backend subproccess exited when trying to invoke build_wheel

If I take the progress indicator "Building wheel from sdist" at face value then I suppose the problem is that challenge-bypass-ristretto does not get included in our sdist so isn't available when this stage is reached.

Maybe addressing this and switching to python -m build would be a reasonable follow-up? Since it appears to be using bdist_wheel under the hood, just like pip wheel does, perhaps the wheels themselves are the same either way and python -m build is only preferable for project maintenance reasons? (In which case a follow-up fix seems fine)

Adding --no-deps seems trouble-free though so that much I can do now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went to file a ticket for using python -m build and I realized I couldn't quite articulate the reason for preferring it. https://pypi.org/project/build/ says it is "a .. package builder" but doesn't try to claim to be the definitive package builder or anything like that. I'm not sure what the pros and cons of it vs pip wheel are. Do you feel like filing this ticket?

Choose a reason for hiding this comment

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

There are a couple of things:

  • partly it is that other tools are not the right choice:
    • directly invoking setup.py is deprecated (see for example this comment and this section of setuptools documentation)
    • pip wheel is intended to get wheels of all dependencies (possibly fetching them from pypi, or building them from sdists)
  • as for why build, it is the suggestion in setuptools' quickstart and by pep517's readme (the pep517 package is the implementation of the new API for building packages which is used by pip).

If I take the progress indicator "Building wheel from sdist" at face value then I suppose the problem is that challenge-bypass-ristretto does not get included in our sdist so isn't available when this stage is reached.

Yeah, it seems that (by default), build builds a sdist, and then builds a wheel from that. That suggests that the sdist that is produced can't actually be installed, which would be a reasonable thing to fix. You can pass -w to build, to get it to just build a wheel directly from source, which works.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
<<: *TESTS
environment:
NIXPKGS_REV: "20.09"
NIXPKGS_REV: "21.05"

Choose a reason for hiding this comment

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

I guess these are tags of the initial release of that branch, which is probably fine for running tests.

@exarkun exarkun merged commit 25d7de1 into master Jan 14, 2022
@exarkun exarkun deleted the support-python3 branch January 14, 2022 14:54
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.

2 participants