-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
build tweaks #58
build tweaks #58
Conversation
# new versions of python don't include distutils. setuptools provides it. | ||
- name: Install setuptools | ||
run: pip install setuptools |
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.
shouldn't need setuptools when only deplying to npm
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 don't recall what the failure was, but i wouldn't have added this unless there was a failure.
- name: Python version | ||
run: python --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.
rather than output the system version, we can look at the output from node-gyp
in the install and build step. this way we confirm the version that node-gyp
is actually using.
- name: Show node-gyp version | ||
run: npm ls node-gyp |
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.
see above, the npm ci
step outputs the node-gyp version that's running. that doesn't always match the version that is installed as a dependency.
uses: actions/download-artifact@v4 | ||
with: | ||
name: prebuilds | ||
pattern: prebuilds-* | ||
path: prebuilds/ | ||
merge-multiple: true |
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.
per the breaking changes to upload v4, this should keep the previous behavior:
downloads prebuilds-linux
, prebuilds-darwin
, and prebuilds-win32
all into the prebuilds/
directory as
prebuilds/
darwin-x64+arm64/
linux-arm64/
linux-x64/
win32-x64/
with: | ||
node-version: 16 | ||
architecture: x64 | ||
node-version: lts/* |
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.
on macos, using anything less than 22 to build for 22 failed locally as well.
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.
makes sense that the node-22 build requires the node-22 includes. thank you!
uses: actions/upload-artifact@v3 | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: prebuilds | ||
name: prebuilds-${{ matrix.build-group }} | ||
path: prebuilds/ |
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.
v4 breaks when you upload multiple artifacts with the same name. see download-artifacts changes for how we handle this.
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.
thanks for this. i think we should add setuptools back in, though what i mostly take from it is that i should have left a comment about what was failing. iirc, node-gyp wants something out of the package, but don't recall what.
https://stackoverflow.com/questions/41216875/what-is-the-purpose-of-python-setuptools
Build using latest lts node.
Updates deprecated actions.