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

feat: added improved gh actions #293

Closed
wants to merge 5 commits into from

Conversation

wadeking98
Copy link
Contributor

@wadeking98 wadeking98 commented May 29, 2024

Added github actions to automatically publish alpha versions to npm.
Included binaries in react native package so we have a smoother release cycle without having to wait for a binary release

This PR does not effect the usual release cycle, this only adds alpha releases. Normal major releases will still have to be triggered manually

Signed-off-by: wadeking98 <[email protected]>
.github/workflows/build.yml Outdated Show resolved Hide resolved
Signed-off-by: wadeking98 <[email protected]>
run:
working-directory: ./wrappers/javascript
timeout-minutes: 7
steps:
Copy link

Choose a reason for hiding this comment

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

A few spaces won't change much but make make it easier for the eye to follow. Between steps?

@@ -54,12 +53,5 @@
"peerDependencies": {
"react": ">= 16",
"react-native": ">= 0.66.0"
},
"binary": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this break the non-alpha releases? My preference would really be to fetch the libraries from gh in a post install (what node-pre-gyp does), but if there are benefits I am okay with it.

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 got rid of this because the new github actions copy the compiled binaries into the native folder, so the alpha release will target the latest binaries

Copy link
Contributor

Choose a reason for hiding this comment

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

we can go back to fetch when/if we can figure out publishing alpha releases of the rust libraries. I was thinking on maybe publishing alphas builds to GitHub Package Registry (maybe as maven artifacts)

Copy link
Contributor

@berendsliedrecht berendsliedrecht Jun 5, 2024

Choose a reason for hiding this comment

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

I got rid of this because the new github actions copy the compiled binaries into the native folder, so the alpha release will target the latest binaries

but it only does this in the alpha-release action, correct? Normal releases will not contain the copied library and fail as we also do not push them to the repository.

we can go back to fetch when/if we can figure out publishing alpha releases of the rust libraries. I was thinking on maybe publishing alphas builds to GitHub Package Registry (maybe as maven artifacts)

I would stay away from maven artifacts, or pods, as it will change the setup process quite a bit. Might be a nice end goal, but for now just including the xcframework and android libraries like so is fine.

For rust alpha releases, something like 0.1.0-alpha.1 will work on crates.io.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly i don't see a reason to change this setup?

It works cross platform for Node.JS / iOS / Android and means you only download the binary for your platform (in the Node.JS release)

We've put quite some thought into this process and I would be really disappointed to see it replaced by something else that caters better to your use case (react native), but doesn't take into account Node.Js

Copy link
Member

Choose a reason for hiding this comment

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

See my suggestion on how to approach this in the other comment

@cvarjao
Copy link
Contributor

cvarjao commented Jun 4, 2024

@andrewwhitehead , @berendsliedrecht , can we get this merged for us to be able to get alpha release for the react native wrapper?

},
"dependencies": {
"@hyperledger/indy-vdr-shared": "0.2.0-dev.6",
"@mapbox/node-pre-gyp": "^1.0.10"
"@hyperledger/indy-vdr-shared": "0.2.0-dev.6"
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs updating to 0.2.2?

@TimoGlastra
Copy link
Member

Included binaries in react native package so we have a smoother release cycle without having to wait for a binary release

It is generally not advised to push such big binaries to the NPM registry. Also because it means you download the binaries for all platforms when you install the package rather than just the binary for your platform.

So from my side this would be a No to merging this PR as is. Maybe we can look at creating pre-releases on github, so that we download the binary and not have to include it in the package.

Comment on lines +438 to +440
needs:
- create-ios-xcframework
- create-android-library
Copy link
Member

Choose a reason for hiding this comment

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

This seems to just release the ios/android wrappers as alpha. Not the Node.JS.

I would really like to keep consistency between node.js and react native.

My proposal for this PR would be:

  • on push to main, build the binaries for indy vdr
  • update the version of the wrapper to the current package.json version but with e.g. .alpha-{commit-sha} appended
  • create a prerelease on github and push the binaries
  • release the JS wrapper

Copy link
Member

Choose a reason for hiding this comment

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

The github pre releaase could e.g. be called "JS 0.2.2 Alpha {sha-commit}" to make it clear this is a JS pre-release.

Then the package.json binary is updated to link to this pre-release.

This way pre-releases work the same as normal releases.

Copy link
Contributor

@cvarjao cvarjao Jun 6, 2024

Choose a reason for hiding this comment

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

It seems messy to manage different lifecycle in the same repository. I think we need to move to uniffi-rs or some other tool. I am concern about those wrappers getting out of sync, missing releases, and release from side branches, like we it was done.

run: |
git update-index --assume-unchanged $(find . -type d -name node_modules -prune -o -name 'package.json' -print | tr "\n" " ")
#export NEXT_VERSION_BUMP=$(yarn next-version-bump)
export NEXT_VERSION_BUMP="major"
Copy link
Member

Choose a reason for hiding this comment

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

This will always increase the version by major, not sure if that's what we want?

Maybe we can just do the current version + alpha-{commit-sha}?

@@ -28,6 +28,7 @@
"ios/indyVdr.xcodeproj/project.pbxproj",
"cpp/**/*.cpp",
"cpp/**/*.h",
"native/**",
Copy link
Member

Choose a reason for hiding this comment

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

This should not be published

@@ -54,12 +53,5 @@
"peerDependencies": {
"react": ">= 16",
"react-native": ">= 0.66.0"
},
"binary": {
Copy link
Member

Choose a reason for hiding this comment

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

Honestly i don't see a reason to change this setup?

It works cross platform for Node.JS / iOS / Android and means you only download the binary for your platform (in the Node.JS release)

We've put quite some thought into this process and I would be really disappointed to see it replaced by something else that caters better to your use case (react native), but doesn't take into account Node.Js

@wadeking98 wadeking98 closed this Jun 6, 2024
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.

6 participants