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

Update node release based on maplibre-js release workflow #1986

Closed
wants to merge 15 commits into from

Conversation

acalcutt
Copy link
Collaborator

@acalcutt acalcutt commented Dec 31, 2023

This PR updates the node release workflow so it no longer increments the version on it's own. This should allow it to work with the approval requirements on this repo. I've made this work more like the maplibre-js workflow at https://github.com/maplibre/maplibre-gl-js/blob/b7b86c1018842a139d4ced21a071ca4a3d9f24b5/.github/workflows/release.yml .

To increment the version now, someone would make a PR that changes the version in package.json located in the root directory (which is used for node). Once that change is made, the workflow automatically checks against the published version on npm. If the version has changed, the binaries get built and published to github using NODE_PRE_GYP_GUTHUB, then the release is published to npm

I've also cherry picked some workflow fixes I made in the opengl-2 branch.

  • update '@acalcutt/node-pre-gyp' to support node 20
  • put '--fallback-to-build=false' in package.json. I had changed this a while back but it should be false since we don't have GYP build files
  • fix 'runner.arch' that should be 'matrix.arch'
  • update readme to reflect binaries being built
  • remove unused node-ci-mac.yml

I was also thinking about versioning. Maybe in the 'opengl-2' branch we can continue a 5.x release , but in main we can start with '6.0.0-pre.0' (once we get something together for a metal pre-release)

Testing
I have created a test release (minus macos support) at
https://github.com/WifiDB/maplibre-native/actions/runs/7368227110/job/20052326170
https://www.npmjs.com/package/@acalcutt/maplibre-gl-native-test/v/5.2.1-pre.9
https://github.com/WifiDB/maplibre-native/releases/tag/node-v5.2.1-pre.9

Wishlist
A way to update release changelog based on https://github.com/maplibre/maplibre-native/blob/main/platform/node/CHANGELOG.md

Notes
After this is approved I plan to cherry pick these changes over to 'opengl-2' so I could also make a updated release with macos support there, so a squash and merge may be easier if this is merged.

@acalcutt acalcutt marked this pull request as ready for review December 31, 2023 05:05
@acalcutt
Copy link
Collaborator Author

acalcutt commented Dec 31, 2023

Any suggestions on how to fix the 'pre-commit.ci' run: error . I looked at other workflows in the repo and they seemed similar.

@louwers louwers self-requested a review January 1, 2024 09:50
@louwers
Copy link
Collaborator

louwers commented Jan 1, 2024

@acalcutt You can run

pre-commit run actionlint

locally or install all pre-commit hooks with

pre-commit install

(after installing pre-commit with pip or another package manager).

I left some suggestions that will solve the issue I think. $(command) needs to be quoted to prevent word splitting (same as with variables).

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Looks great!

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jan 2, 2024

I changed this a little bit so instead of checking if the current version matched the latest version published, it just checks if the version is published.

In testing I realized If I published a newer version on a different branch (like main), it would break publishing of lower version releases (like I want to do with the opengl-2 branch). Just checking if it is published will allow simultaneous releases and allow it to only publish when needed still.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jan 2, 2024

i still haven't figured out that last pre-commit.ci error, it still doesn't like line 27 but it seems like most of it is in quotes now...

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jan 4, 2024

@louwers I was wondering what you think about this.

In a test branch I added code that adds the node changelog to these releases. This is also based on the maplibre-js release workflow. It takes the version change information from 'platform/node/CHANGELOG.md' and Updates the release with the text for the version posted.
https://github.com/WifiDB/maplibre-native/blob/node_pr_workflow_test/.github/workflows/node-release.yml#L271-L304

This needs this script i modified from maplibre-js, like this one
https://github.com/WifiDB/maplibre-native/blob/node_pr_workflow_test/scripts/release-notes-node.js

I also had to fix the indentation #'s in 'platform/node/CHANGELOG.md' to match what maplibre-js used
https://raw.githubusercontent.com/WifiDB/maplibre-native/node_pr_workflow_test/platform/node/CHANGELOG.md

Do you think these changes are an improvement? should I merge them in here?

@louwers
Copy link
Collaborator

louwers commented Jan 4, 2024

Do you think these changes are an improvement? should I merge them in here?

Sure, let's try it out. Maybe in a different PR?

Can you put the script in platform/node/scripts (since it is Node.js specific)?

Can you error when no release notes are found?

@louwers
Copy link
Collaborator

louwers commented Jan 5, 2024

@acalcutt I am using this awk one-liner now for iOS:

awk '/^##/ { p = 0 }; p == 1 { print }; $0 == "## {{ env.version }}" { p = 1 };' CHANGELOG.md > changelog_for_version.md

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jan 6, 2024

I am going to close this in favor of #1995 . I have moved that PR to use awk like recommended

@acalcutt acalcutt closed this Jan 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.

2 participants