-
Notifications
You must be signed in to change notification settings - Fork 14
Improve build for packaging, add release workflow in CI #212
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
base: main
Are you sure you want to change the base?
Improve build for packaging, add release workflow in CI #212
Conversation
| @@ -0,0 +1,42 @@ | |||
| # Releasing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document when to use release-v0.2.0-alpha, release-v0.2.0-beta, release-v0.2.0-rc? Do we plan on having a stable and unstable branch in the future? It seems like the workflow changes are laying the groundwork for the latter, but I didn't see any explicit mention of it. I bring this up only for documentation purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions!
Should we document when to use release-v0.2.0-alpha, release-v0.2.0-beta, release-v0.2.0-rc?
This feels more like an internal discussion, but my initial take is that dist-tags aren't really necessary until there's a major release. The other argument though is to have a formal process right away and treat minor versions similar to major (until there's a major release) i.e. release-v0.1.0-rc0... rc1... -> release-v0.1.0
Do we plan on having a stable and unstable branch in the future?
I think it makes sense in the future (if not now) with audits, bug bounties, experimental features, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels more like an internal discussion, but my initial take is that dist-tags aren't really necessary until there's a major release. The other argument though is to have a formal process right away and treat minor versions similar to major (until there's a major release) i.e. release-v0.1.0-rc0... rc1... -> release-v0.1.0
I'm thinking we can put this off until we get closer to Compact 1.0
I think it makes sense in the future (if not now) with audits, bug bounties, experimental features, etc
I'm down to get this ball rolling sooner rather than later
Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]> Signed-off-by: Andrew Fleming <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Approving, but we should wait for Son's review before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to set up branch protection rule for the release-v* branch
| # Publish the tarball with appropriate tag | ||
| npm publish "${{ steps.pack.outputs.tarball }}" --tag "${{ steps.pack.outputs.tag }}" --access public | ||
| env: | ||
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to switch this over to Trusted Publishing later. I am tracking that on my side.
| mkdir -p "$COMPACT_ZIP_DIR" | ||
| ZIP_FILE="compactc_v${COMPILER_VERSION}_x86_64-unknown-linux-musl.zip" | ||
| DOWNLOAD_URL="https://d3fazakqrumx6p.cloudfront.net/artifacts/compiler/compactc_${COMPILER_VERSION}/${ZIP_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to extract these out to Actions variables instead of hardcoding, can be a separate improvement PR later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also would like to see some verification for the downloaded file, like a SHA256 hash or something.
|
|
||
| - name: Extract current version | ||
| run: | | ||
| CURRENT_VERSION=$(node -p "require('./contracts/package.json').version") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if nodejs is available in the runner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is supported in the runner but I will add this part to be safe:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version-file: ".nvmrc"
cache: "yarn"
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #134.
Waiting on #210.
PR Checklist
Overview
setupPackaging
The build logic creates a
distdirectory and copies the repo structure but removessrc/from the path. Therefore, the path fromcontracts/looks like this:contracts/dist/access/...Each subdirectory includes the associated Compact modules and
witnesses/. The files withinwitnesses/are compiled to support .ts and .js (no source maps)This packaging allows users to import compact modules and witnesses like this:
import "./node_modules/@openzeppelin-compact/contracts/access/AccessControl" prefix AccessControl_;import "./node_modules/@openzeppelin-compact/contracts/access/witnesses/AccessControlWitnesses";Release flow
The release process is more thoroughly documented in RELEASING.md, but here's the condensed flow:
mainunless hotfix).mainfor manual review + approval.Other notes
actions/setup/action.ymlprivateto allow publishingcontractsmanifest metadata #149