-
Notifications
You must be signed in to change notification settings - Fork 1
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 build system for Linux, macOS, add GitHub Actions #30
base: main
Are you sure you want to change the base?
Update build system for Linux, macOS, add GitHub Actions #30
Conversation
Update GYP to build for Linux and macOS platforms, as well as ARM and ARM64 for Linux. Updated script to download CANBridge for Linux and macOS, including ARM platforms for Linux.
Added GitHub Actions workflows for building and releasing node-can-bridge. This can run specific tasks automatically to ensure builds are operational. This also includes making releases via pushing tags. Added Dependabot to update workflow Actions to ensure they are up to date to negate any security issues and deprecation issues with older Actions.
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.
Hi @garrettsummerfi3ld, I'm planning to convert this project to cmake-js (away from node-gyp-build). This primarily gives us wider IDE support and avoids some of the headaches I've had with gyp in the past. Since I'm making this change already, I'd be happy to take other platforms into consideration. Unfortunately, I'm only set up to test my changes on Windows and Linux, so I'll make a best-effort for macOS, but I can't test it. When my PR is merged, y'all should only have to pull in my changes (merge or rebase), remove binding.gyp, and keep your workflow changes. I can help with any issues that come up for the Linux build, but again, macOS will have to be best-effort from me since I can't test.
I can test macOS changes as they come up. With cmake-js, is there any major build considerations needed to make sure that other platforms are well supported? I don't see a branch for it yet but once I see that merged into |
CMake handles different platforms well on its own. There should actually be fewer differences (no separate msvc and xcode settings). If you're not planning to cross-compile, I'd expect builds to mainly Just Work (tm) on the different platforms. |
One note is that I'm using the VSCode generator on Windows now, which keeps the build/Release folder. I don't think others will. They'll hold the binary in build. |
I believe the new prebuild tool (pkg-prebuild) wants to place the binaries in 'prebuilds/{package_name}-{platform}' |
I just tested with the latest commit of my cmake-js branch on Linux. It builds the binary just fine when running 'npm install' (when I copy the download script from this branch, and update it to use unofficial-rev-port/CANBridge 2.4.0, we don't publish Linux binaries on ours yet, and y'all haven't updated to 2.5.0 yet). When I run the test, it fails with a linker error, but it looks like this branch will too from my testing if you make the same change to use unofficial-rev-port/CANBridge 2.4.0. |
Refactored 'runtimeArtifactsPath' to use 'path' instead of manual entry, which might break on some platforms. Also updated names of prebuild artifacts.
Merged from upstream to make changes, at this moment I am unable to test if those changes are working as I am waiting for a dependent PR to be merged. |
downloadCanBridgeArtifact('wpiutil.dll', runtimeArtifactsPath), | ||
downloadCanBridgeArtifact('headers.zip', tempDir), | ||
)); | ||
// TODO: Do not hardcode the filenames, instead get them from the GitHub API -> Look at Octokit: https://github.com/octokit/octokit.js |
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.
Octokit is great if you need it, but I don't really see the problem with hardcoding URLs here. We need to hardcode a specific tag, so using Octokit to tell us what the latest version is wouldn't be useful. Octokit could give us a full list of artifacts, but we'd still need to hardcode which ones we want to use (unless we wanted to try to use some sort of heuristics to figure out which file contains what we're looking for, which feels unnecessary). What do you see as the advantages of using Octokit in this context?
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 what I was after was to have it always pull from latest and to iterate through every single asset that was in a release, rather than hardcoding the files and the version to be specific on what was needed to be downloaded. Memory on what that was specifically for is a bit hazy. In my mind it would cut down on commits updating the tag as well as not having to worry about missing artifacts if I just download them all from an endpoint.
Little more complex, but would be more extendable as far as what could be available in the future. This note can be totally tossed out if we don't care about those commits or hardcoding. More of a "hey check this out, might help with something in the future" kind of thing.
This PR introduces a number of changes to the build system and adds GitHub Actions to automate the building and releasing process of this repository.
Note
This PR is marked as a draft as this is blocked until REVrobotics/CANBridge#37 is merged. The build system changes and release workflows for CANBridge on that PR directly influences this PR.