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

Stubs upload actions #4861

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Stubs upload actions #4861

merged 4 commits into from
Jun 21, 2021

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Jun 5, 2021

This adds some steps in the actions workflows to upload the generated stubs to pypi.

It seems to fail for something about an invalid version number when it runs on my fork. But I was able to successfully test it by changing the version string to be hard coded instead of generated dynamically inside of setup.py I'm hopeful this issues is related to being a separate fork on the repo and won't be a problem on this real main branch.

The test pypi listing for the successful test can be found here for now:

pip install circuitpython-stubs-foamyguy-1

This is the error message from my fork when it does fail using the generated version string:

HTTPError: 400 Bad Request from https://upload.pypi.org/legacy/
100%|██████████| 101k/101k [00:00<00:00, 247kB/s]
NOTE: Try --verbose to see response content.
'7.0.0a3.dev16+g5a22a0f60' is an invalid value for Version. Error: Can't use PEP 440 local versions. See https://packaging.python.org/specifications/core-metadata for more information.
Error: Process completed with exit code 1.

@lesamouraipourpre
Copy link

Following today's stream I've been thinking more about this and researching PEP440 and think the following changes should be made.

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 4a478c381..47813766b 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -63,14 +63,6 @@ jobs:
       run: |
         python -m pip install --upgrade pip
         pip install setuptools wheel twine
-    - name: Publish stubs
-      if: contains(steps.need-pypi.outputs.setup-py, 'setup.py')
-      env:
-        TWINE_USERNAME: ${{ secrets.pypi_username }}
-        TWINE_PASSWORD: ${{ secrets.pypi_password }}
-      run: |
-        python setup.py sdist
-        twine upload dist/*
     - name: Test Documentation Build (HTML)
       run: sphinx-build -E -W -b html -D version=${{ env.CP_VERSION }} -D release=${{ env.CP_VERSION }} . _build/html
     - uses: actions/upload-artifact@v2
@@ -126,6 +118,13 @@ jobs:
         name: mpy-cross.static-x64-windows
         path: mpy-cross/mpy-cross.static.exe
     - name: Upload stubs and mpy-cross builds to S3
+      if: github.event_name == 'push' || (github.event_name == 'release' && (github.event.action == 'published' || github.event.action == 'rerequested'))
+      env:
+        AWS_PAGER: ''
+        AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
+        AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
+        TWINE_USERNAME: ${{ secrets.pypi_username }}
+        TWINE_PASSWORD: ${{ secrets.pypi_password }}
       run: |
         [ -z "$AWS_ACCESS_KEY_ID" ] || aws s3 cp mpy-cross/mpy-cross.static-aarch64 s3://adafruit-circuit-python/bin/mpy-cross/mpy-cross.static-aarch64-${{ e
nv.CP_VERSION }} --no-progress --region us-east-1
         [ -z "$AWS_ACCESS_KEY_ID" ] || aws s3 cp mpy-cross/mpy-cross.static-raspbian s3://adafruit-circuit-python/bin/mpy-cross/mpy-cross.static-raspbian-${{ env.CP_VERSION }} --no-progress --region us-east-1
@@ -133,11 +132,10 @@ jobs:
         [ -z "$AWS_ACCESS_KEY_ID" ] || aws s3 cp mpy-cross/mpy-cross.static.exe s3://adafruit-circuit-python/bin/mpy-cross/mpy-cross.static-x64-windows-${{ env.CP_VERSION }}.exe --no-progress --region us-east-1
         zip -9r circuitpython-stubs.zip circuitpython-stubs
         [ -z "$AWS_ACCESS_KEY_ID" ] || aws s3 cp circuitpython-stubs.zip s3://adafruit-circuit-python/bin/stubs/circuitpython-stubs-${{ env.CP_VERSION }}.zip --no-progress --region us-east-1
-      env:
-        AWS_PAGER: ''
-        AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
-        AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
-      if: github.event_name == 'push' || (github.event_name == 'release' && (github.event.action == 'published' || github.event.action == 'rerequested'))
+        if [[ -z "${{steps.need-pypi.outputs.setup-py}}" && -z "$TWINE_PASSWORD" ]]; then
+          python setup.py sdist
+          twine upload dist/*
+        fi
 
 
   mpy-cross-mac:
diff --git a/setup.py b/setup.py
index 8c1f27e40..2c898a88b 100644
--- a/setup.py
+++ b/setup.py
@@ -20,8 +20,7 @@ version = git_out.strip().decode("utf-8")
 # Detect a development build and mutate it to be valid semver and valid python version.
 pieces = version.split("-")
 if len(pieces) > 2:
-    # Merge the commit portion onto the commit count since the tag.
-    pieces[-2] += "+" + pieces[-1]
+    # Dump the github hash part
     pieces.pop()
     # Merge the commit count and build to the pre-release identifier.
     pieces[-2] += ".dev." + pieces[-1]

The changes to build.yml move the upload to PyPI to the same step as the upload to S3. We shouldn't try until everything else has passed. It will only attempt the PyPI upload if setup.py was found (currently always true) and the pypi_password secret was found (only true on the adafruit fork)

The change in setup.py should generate a valid PEP440 version number which will allow the upload to PyPI to work as long as the pypi_password is valid.

@tannewt tannewt self-requested a review June 7, 2021 22:05
@tannewt tannewt added this to the 7.0.0 milestone Jun 7, 2021
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Will this release to pypi every push? Ideally we'd have two pypi releases at any time, the stable one and the pre-release one. Otherwise I worry about API mismatches.

@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Jun 7, 2021

I do think it would run on every push currently. I'm still very green with github actions, I'm not certain how it specifies what the trigger should be. I think most libraries have a release.yml that gets triggered when releases are made. Perhaps we need that here as well?

I'm also not sure about how to separate out the stable version from the pre-release one if they execute the make stubs in different branches by default it may "just work" without needing to set up anything specific for it. Would we need to use a different name for one of the postings as well? like the stable one be pip install circuitpython-stubs and the pre release one be pip install circuitpython-stubs-pre-release?

@lesamouraipourpre
Copy link

Will this release to pypi every push? Ideally we'd have two pypi releases at any time, the stable one and the pre-release one. Otherwise I worry about API mismatches.

It pushes to PyPI every time there is an upload to S3, so I believe every time a PR gets merged.
It should push a 'RELEASE' version which will match the curated releases and a 'DEV' version which will match the other uploads being pushed to S3. Due to PyPI and PEP440 requirements the github hash cannot be part of the version so it is just based on the commit count part, eg 7.0.0a3.dev16

AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
if: github.event_name == 'push' || (github.event_name == 'release' && (github.event.action == 'published' || github.event.action == 'rerequested'))

if [[ -z "${{steps.need-pypi.outputs.setup-py}}" && -z "$TWINE_PASSWORD" ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am thinking this if statement logic might need to be tweaked. it might also be good to add an else clause that prints a message about being skipped if it does evaluate to false.

I tried merging this over to foamyguy/main (with my own PyPi project name) but as far as I can tell from the actions output seems like it did not do the upload. My best guess is that this came out false and caused it to skip. tonight or tomorrow I will try to tinker some more to add some debugging output to try to make it easier to tell what is going on.

Here are the actions output: https://github.com/FoamyGuy/circuitpython/runs/2769102086?check_suite_focus=true

@Neradoc
Copy link

Neradoc commented Jun 8, 2021

Will this release to pypi every push? Ideally we'd have two pypi releases at any time, the stable one and the pre-release one. Otherwise I worry about API mismatches.
It pushes to PyPI every time there is an upload to S3, so I believe every time a PR gets merged.

Note that currently local branches commits (pushes) pass the if test for uploads to S3: #4632 This happens when someone (with write access) accidentally makes a PR via a local branch in the repo (as opposed to their own fork), before the PR is merged or even submitted, since the branch is created before the "Open a pull request" page is loaded.

I do think it would run on every push currently. I'm still very green with github actions, I'm not certain how it specifies what the trigger should be. I think most libraries have a release.yml that gets triggered when releases are made. Perhaps we need that here as well?

I'm new to that too, but I believe that at the very start of the file this makes all jobs trigger on all pushes and PRs (and releases). (Incidentally it also causes some builds to be doubled, but that doesn't impact the upload).

on:
  push:
  pull_request:
  release:
    types: [published]
  check_suite:
    types: [rerequested]

While the if in each job's "upload to S3" restricts it back to merged PRs (pushes) and releases (plus what I mention in the issue above).

@tannewt
Copy link
Member

tannewt commented Jun 8, 2021

I do think it would run on every push currently. I'm still very green with github actions, I'm not certain how it specifies what the trigger should be. I think most libraries have a release.yml that gets triggered when releases are made. Perhaps we need that here as well?

I'm also not sure about how to separate out the stable version from the pre-release one if they execute the make stubs in different branches by default it may "just work" without needing to set up anything specific for it. Would we need to use a different name for one of the postings as well? like the stable one be pip install circuitpython-stubs and the pre release one be pip install circuitpython-stubs-pre-release?

I think you'll want to split the S3 upload from the pypi upload and have a different if condition for pypi. pypi also has a way to distinguish stable vs dev but I'm not sure how. You can cheat the determination by assuming anything with a - is pre-release. Stable releases will just be numbers and periods.

@FoamyGuy
Copy link
Collaborator Author

The latest commits separate out the pypi upload the mpy-cross s3 upload and use setuptools_scm to generate the version string which is the same utility used by circuit python libraries.

I have successfully tested both dev and stable uploads using this branch: https://github.com/FoamyGuy/circuitpython/tree/foamyguy_test_stubs which had a modified package name that included my username.

I think once pypi credentials are added to this repo these actions should begin working to upload the stubs to PyPi.

@tannewt
Copy link
Member

tannewt commented Jun 21, 2021

I think once pypi credentials are added to this repo these actions should begin working to upload the stubs to PyPi.

I think they are set at the organizational level so they'll already apply.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for splitting the two! I'm going to merge now and we can see how things go. Thank you for putting this together!

@tannewt tannewt merged commit 44d471a into adafruit:main Jun 21, 2021
@FoamyGuy
Copy link
Collaborator Author

looks like we got 403 from PyPi:

Uploading circuitpython-stubs-7.0.0a4.dev82.tar.gz

  0%|          | 0.00/8.94M [00:00<?, ?B/s]
  0%|          | 8.00k/8.94M [00:00<03:14, 48.2kB/s]
  3%|▎         | 264k/8.94M [00:00<00:08, 1.14MB/s] 
 17%|█▋        | 1.55M/8.94M [00:00<00:01, 5.50MB/s]
 79%|███████▉  | 7.06M/8.94M [00:00<00:00, 21.2MB/s]
100%|██████████| 8.94M/8.94M [00:00<00:00, 9.60MB/s]
NOTE: Try --verbose to see response content.
HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/
The user '***' isn't allowed to upload to project 'circuitpython-stubs'. See https://pypi.org/help/#project-name for more information.
Error: Process completed with exit code 1.

Maybe it was created with a different account? I noticed on PyPi the current circuitpython-stubs listing shows author John Reese with you as a maintainer:
image

On a different circuitpython library PyPi listing it shows author as Adafruit
image

@jepler
Copy link
Member

jepler commented Jun 22, 2021

A side note, I'm a bit surprised that the stubs are nearly 9MB. Do they have extra content they shouldn't have? Are they more than just some ".pyi" files?

Comment on lines +12 to 14
STD_PACKAGES = set(('array', 'math', 'os', 'random', 'struct', 'sys', 'ssl', 'time'))

STD_PACKAGES = set(('array', 'math', 'os', 'random', 'struct', 'sys', 'ssl', 'time'))
Copy link
Member

Choose a reason for hiding this comment

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

this looks funky

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yep I think this got doubled up, it's likely I misclicked it while working through changes to merge from main. I'll make a new PR to remove the dupe

@FoamyGuy
Copy link
Collaborator Author

When it runs it does print a bunch of other unrelated files, many things that are in the circuitpython repo, but not stubs specifically.

If you unfold the stubs step here: https://github.com/adafruit/circuitpython/runs/2879766210?check_suite_focus=true

it seems to take a second to load. For me it's a giant blank section for a while but eventually gets filled in if I scroll around a bit.

It does list many files that are unrelated to stubs:

...
creating circuitpython-stubs-7.0.0a4.dev82/ports/atmel-samd/boards/feather_m0_supersized
creating circuitpython-stubs-7.0.0a4.dev82/ports/atmel-samd/boards/feather_m4_can
creating circuitpython-stubs-7.0.0a4.dev82/ports/atmel-samd/boards/feather_m4_express
creating circuitpython-stubs-7.0.0a4.dev82/ports/atmel-samd/boards/fluff_m0
creating circuitpython-stubs-7.0.0a4.dev82/ports/atmel-samd/boards/gemma_m0
...

As far as I could tell these did not seem to be getting included when the stubs are installed via pip though. Looking in the site-packages directory of the virtaul env I've been testing with I see only relavent dirs and pyi files, none of these extras that get printed during the action run. Perhaps they are in there somewhere but in a less obvious location.

@tannewt
Copy link
Member

tannewt commented Jun 22, 2021

@FoamyGuy I've added adafruit-travis as a maintainer. @jreese started this project at a Pycon sprint which is why they are an owner.

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.

None yet

5 participants