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 build system for Linux, macOS, add GitHub Actions #30

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

garrettsummerfi3ld
Copy link

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.

  • 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.
  • Added Dependabot to update workflow Actions to ensure they are up to date to negate any security issues and deprecation issues with older Actions.

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.

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.
Copy link
Contributor

@LandryNorris LandryNorris left a 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.

@garrettsummerfi3ld
Copy link
Author

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 main or something I will look at the implementation and ensure our build steps and workflows work before we do anything else.

@LandryNorris
Copy link
Contributor

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.

@LandryNorris
Copy link
Contributor

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.

@LandryNorris
Copy link
Contributor

I believe the new prebuild tool (pkg-prebuild) wants to place the binaries in 'prebuilds/{package_name}-{platform}'

@LandryNorris
Copy link
Contributor

LandryNorris commented Oct 2, 2024

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.
@garrettsummerfi3ld
Copy link
Author

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
Copy link
Member

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?

Copy link
Author

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.

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.

3 participants