Upload wasm32 python wheel to release
#2279
Conversation
texodus
changed the title
wasm32 python wheel to release
.github/workflows/build.yml
Outdated
| python/perspective/dist/*.tar.gz | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
There was a problem hiding this comment.
This isn't the canonical publish artifact for these (pypi), and we don't use them in any workflows. In that sense, this change exclusively creates more work; it's a thing that can break that we don't use or have tests for, create confusion or distrust (if we break this in the future without noticing and publish different versions to pypi and GH releases).
It also can't be said to be more consistent, as we're not releasing all of the artifacts, just the Python wheels.
We could however use these for publishing, which would guarantee the artifacts are the same and preserve the originals (since GitHub Actions expire eventually). To do this, you'd need to:
- Update publish.mjs to download artifacts from a release instead of a build.
- Upload all artifacts (including NPM artifacts) to release.
CHANGELOG.md
Outdated
| @@ -1,3 +1,17 @@ | |||
| # [v2.3.1](https://github.com/finos/perspective/releases/tag/v2.3.1) | |||
There was a problem hiding this comment.
You rebased on a reverted commit. Drop this commit please 76913ac.
timbess
force-pushed
the
feature/add-python-wheels-to-release
branch
from
415e0a1 to
ae7b348
Compare
timbess
force-pushed
the
feature/add-python-wheels-to-release
branch
from
ae7b348 to
7bf871d
Compare
timbess
force-pushed
the
feature/add-python-wheels-to-release
branch
from
5ca2f2f to
e1b90be
Compare
There was a problem hiding this comment.
This is the comment from the previous review:
- Update publish.mjs to download artifacts from a release instead of a build.
- Upload all artifacts (including NPM artifacts) to release.
This is the equivalent descriptor for what this PR does:
- Update
publish.mjsto upload artifacts to a github release and also pypi+npm.- Call
publish.mjsfrom the Workflowbuild.ymlin CI
This isn't quite what I wanted from this change, but I'm in a hurry to get the wasm32 disted so I let's try it. However there are also some blockers still:
- There are a few hard errors in this PR (looks like rebase errors).
- I'm quite skeptical, when I do run this on CI, that this task will be able to run
publish.mjssuccessfully anyway, as I'm not sure theoctokitworkflow-downloading API can be used form within the workflow it is attempting to download from ... I suspect this needs to use the workflow'sactions/download-artifactstep instead? - We can't publish to NPM or Pypi from CI yet.
- For the other wheel sit doesn't matter, but for the pyodide wheel a
.zipdoesn't help us - we need the unzipped.whldisted directly for it to work as a web host for this artifact form pyodide.
In the interest of getting this working quickly, I'm going to go ahead and close this PR and pick up these changes in #2294 which has been rebased and implemented a few fixes necessary to start testing
| @@ -17,6 +17,10 @@ import { promises as fs } from "fs"; | |||
| import { Octokit } from "octokit"; | |||
| import readline from "readline"; | |||
|
|
|||
| const CURRENT_TAG = execSync("git describe --exact-match --tags") | |||
There was a problem hiding this comment.
execSync is a missing symbol - did you check this file after rebasing?
| repo: "perspective", | ||
| tag_name: CURRENT_TAG, | ||
| name: CURRENT_TAG, | ||
| body: "Release of perspective", |
There was a problem hiding this comment.
Better IMO to remove this, we know it's a release of perspective, this isn't the DMV. Also, lets make these drafts so we can opt into publishing them?
| @@ -207,6 +216,23 @@ if (proceed.toLowerCase() === "y") { | |||
| recursive: true, | |||
| }); | |||
|
|
|||
| const release = await octokit.request( | |||
There was a problem hiding this comment.
COMMIT flags is a safety bool to prevent this script from having side effects unless intended. When run without COMMIT, I expect this script to run but not actually publish in the end, so I was quite surprised when I ran this for this first time and it published a GitHub release.
Since this script is now being called from CI, the release-to-GitHub functionality in this script either needs to go behind the same COMMIT flag (see bottom of this script), or a separate flag just for CI.
| CI: true | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GITHUB_WORKFLOW_ID: ${{ github.run_id }} | ||
| COMMIT: ${{ github.sha }} |
There was a problem hiding this comment.
This isn't the commit sha, it's a flag to tell publish.mjs whether to run in dry-run mode. Setting this to SHA causes this script (in CI) to attempt to publish to pypi & npm, which it isn't authenticated for (and we don't want to do this for now also).
| `${dist_folder}/${artifact.name}.zip`, | ||
| Buffer.from(download.data) | ||
| ); | ||
| } | ||
| }) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Need the same checks for pyodide everywhere python_dist_folders is iterated over, since the pyodide artifact is in this list but not on the disk due to the conditional branch exception above.
This will allow us to keep a history of all Python builds with our Github Releases. This is mostly so that we can host the WASM wheels, but while I was in there I also added the other wheels. I can remove that if we want, but I figured it wouldn't hurt to just host all of the Python artifacts on Github as well.
Before release
We'll need to setup a Github secret with a personal access token with write permissions for this to work:
https://github.com/softprops/action-gh-release/tree/master#permissions