Skip to content

Update build system for Linux, macOS, overhaul GitHub Actions, remove serial driver #37

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

Merged

Conversation

garrettsummerfi3ld
Copy link
Contributor

@garrettsummerfi3ld garrettsummerfi3ld commented Sep 23, 2024

This PR introduces a number of changes to the build system and GitHub Actions workflows, as well as removing the unused serial driver.

  • Overhauled GitHub Actions workflows to include Linux, macOS platforms, alongside concurrency for builds and split larger steps into smaller steps for legibility and debugging of failing actions.
    • New runners include X86, ARM32, and ARM64 Linux runners, macOS runners compiling universal binaries alongside the existing Windows runner. ARM runners are using a WPI provided Docker container to build against.
    • Added special rulesets for workflow concurrency for jobs running in sync and queueing builds for branches.
    • Added special rulesets for allowing all branches to run actions instead of only main and PRs to main for testing builds on multiple platforms before a PR.
    • Split apart larger tasks into smaller tasks for better verbosity during workflow runs.
    • Updated action dependencies to latest as older actions are deprecated due to old Node.js runtimes.
    • Restructure how artifacts are created, where instead of a single large artifact including CANBridge binaries and WPILib headers, it is now separated into its own artifact as the headers are universal across all platforms.
    • Cleaned up release workflow for pushing tags to create a draft release where instead of using the GitHub CLI, a dedicated action is used instead for simplicity.
  • Updated GoogleTest to fix an issue with macOS utilizing incorrect platform from the WPILib Artifactory. This would fail macOS builds completely.
  • Added IOKit linker args for macOS builds to use with the serial driver.
  • Removed Windows-only preprocessor macros on the serial driver for all platforms.
  • Removed serial driver completely. This driver was not used, nor supported or maintained.
  • Add GitHub Dependabot to automate updating GitHub Actions.

There are some changes made in this PR that breaks builds for node-can-bridge, plans to update build and CI for node-can-bridge are underway to incorporate Linux and macOS builds for future releases.

PR that addresses these concerns is created at REVrobotics/node-can-bridge#30 for discussion and review.

@garrettsummerfi3ld garrettsummerfi3ld marked this pull request as ready for review September 23, 2024 20:18
build.gradle Outdated
@@ -78,6 +78,9 @@ model {
}
}
binaries.all {
if (it.targetPlatform.name == 'osxuniversal') {
linker.args '-framework', 'IOKit'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary with the removal of the serial driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not necessary as the serial driver is removed. However, building back with SocketCAN or some other framework with CAN would be beneficial to leave in. There is no hurting with keeping it here.

Copy link
Contributor

@ItsHarper ItsHarper Nov 12, 2024

Choose a reason for hiding this comment

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

Is that true if we end up using DriverKit instead of IOKit? According to this page, DriverKit is an IOKit replacement: https://developer.apple.com/system-extensions/

DriverKit provides a fully modernized replacement for IOKit to create device drivers.

Copy link
Contributor

@ItsHarper ItsHarper Nov 12, 2024

Choose a reason for hiding this comment

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

USBDriverKit certainly sounds like what we want: https://developer.apple.com/documentation/usbdriverkit

Copy link
Contributor

@ItsHarper ItsHarper Nov 12, 2024

Choose a reason for hiding this comment

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

Looking at the transcript of the video in that first link, there's a lot of emphasis on DriverKit being better than kernel extensions, which I'd think never would have been necessary in the first place for what we're doing.

It seems like DriverKit would require a separate System Extension that would handle all direct communication with the USB device, and then CANBridge would communicate with our DriverKit driver. That would be fine, but I think we'd have to design a whole API between our driver and CANBridge, which does sound like more work.

IOKit (and specifically IOUSBLib.h) on the other hand looks like a model more similar to what we're doing on Windows (raw USB transactions through WinUSB). The "Important" box on this page makes me a bit nervous, but it's really not clear what it's trying to say:

Devices supported on macOS 11 and later require DriverKit. Use IOKit in your apps and services to discover and use devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this in the DriverKit presentation transcript:

The DriverKit APIs are an extension of the IOKit APIs to userspace. And we have collected them into a new DriverKit SDK that is separate to the macOS SDK. This SDK has a limited API surface for reliability and security and there is no direct access to the file system, networking, or mock messaging. This allows Apple to tailor the userspace process to running drivers and can give it elevated priority and increased capabilities. So, let's talk about some of the classes in the DriverKit SDK. First, the IOService class exists in DriverKit and is very similar to the IOKit class. There also IOMemoryDescriptor and IOBufferMemoryDescriptor classes available that are, again, very similar to IOKit. We also have replacements for the IOWorkLoop and EventSource classes in IOKit. And finally, there's a new class called OSAction that is required to represent a C Function Pointer.

So yeah, I think IOKit is probably the way to go if we don't need any kind of extra OS-level permissions like a kext would have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So from reading about macOS drivers as a whole when I made the PR, its a mess at the moment on which framework to use.

IOKit is only available on macOS but is being given the "deprecated but not deprecated" treatment. Those drivers run as kernel extensions and because in the name of security and stability those are getting moved over to DriverKit which solves the security and stability issues. Lots of documentation for how to do things since its been around for so much longer, but Apple has other ideas on what to do.

If we want maintained support for the future, IOKit will be discouraged from developing for in favor of using DriverKit.

Could be totally misinterpreting this as there was a whole macOS update that happened that added more things to DriverKit that I haven't dived into the docs for but the writing on the wall looks to be there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dropped for now. We can re-add this if/when we actually have a macOS CAN driver.

@ItsHarper
Copy link
Contributor

ItsHarper commented Sep 23, 2024

Thanks for making this (at first glance) so much easier to review! Going to try to get to this this week.

@garrettsummerfi3ld
Copy link
Contributor Author

This PR had downstream issues with building node-can-bridge, the PR that addresses this concern has been created here: REVrobotics/node-can-bridge#30

Copy link
Contributor

@ItsHarper ItsHarper left a comment

Choose a reason for hiding this comment

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

Finally got started looking at this in depth! So far, I've got a request to reduce code duplication, and a PR that will add static WPILib dependencies to the artifacts instead of just dynamic ones (unofficial-rev-port#8). I'd strongly recommend reviewing and merging that PR before creating assemblePlatformSpecificArtifacts.sh.

@qwertychouskie qwertychouskie force-pushed the fix/upstream-build-changes branch from 8265f52 to 2fa667d Compare November 24, 2024 06:21
@qwertychouskie
Copy link
Contributor

In addition to fixing the code duplication issue, I also just rebased the PR. Unless any new issues are found, this should be ready to merge.

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.

Some comments added, but I'm ok with this as-is also.

@LandryNorris
Copy link
Contributor

@slyboots I know you usually don't touch the C code, but let's give you the chance to look over this if you want before merging.

@qwertychouskie qwertychouskie force-pushed the fix/upstream-build-changes branch from f998a82 to 6d4c7a2 Compare May 28, 2025 03:40
Updated workflows from upstream main to include macOS, Linux, and ARM variants to build against. Added concurrency for the workflows and also split some workflows out for smaller individual artifact sizes. Updated actions to latest available. Changed build action to run on every commit pushed rather than only PR's and pushes to main.

Updated GoogleTest dependency to resolve issue with the WPILib Artifactory, this was causing issues with builds on macOS where osxuniversal platform type was not available, and split between osxarm64 and osxx86-64.
@qwertychouskie qwertychouskie force-pushed the fix/upstream-build-changes branch from 6d4c7a2 to 3f33857 Compare May 28, 2025 03:48
@qwertychouskie
Copy link
Contributor

Sorry for the commit noise, the commit list should be clean now. I squashed the newline addition into the commit that added that file, and dropped the commit that made a trivial change to the serial driver that gets completely removed a couple commits later anyways.

@qwertychouskie qwertychouskie force-pushed the fix/upstream-build-changes branch from 3f33857 to 360668f Compare May 28, 2025 04:39
garrettsummerfi3ld and others added 6 commits May 27, 2025 21:48
This check ensures that the tag pushed and the Gradle version are the same, and if the two don't match, then CI will fail.
The serial driver has been stated for removal for some time now, this removed the driver completely.
Dependabot keeps repo dependencies updated. For this configuration, we are just updating GitHub Actions. This can extend into Gradle if needed but this is just to keep individual Actions updated.
@qwertychouskie qwertychouskie force-pushed the fix/upstream-build-changes branch from 360668f to a06a0bb Compare May 28, 2025 04:49
@slyboots
Copy link

YOOOOOOOOO! 🚢 🇮🇹

@slyboots slyboots dismissed ItsHarper’s stale review May 28, 2025 15:45

no longer works here

@LandryNorris
Copy link
Contributor

@garrettsummerfi3ld this is good to merge when you're ready

@garrettsummerfi3ld
Copy link
Contributor Author

@LandryNorris I don't have the ability to merge.

@LandryNorris
Copy link
Contributor

I can merge it. Is it good on your side?

@LandryNorris LandryNorris merged commit 054420c into REVrobotics:main Jun 18, 2025
7 checks passed
@qwertychouskie qwertychouskie deleted the fix/upstream-build-changes branch June 18, 2025 20:03
@qwertychouskie
Copy link
Contributor

@LandryNorris Thanks for getting this merged! Can you tag a new release of CANBridge so REVrobotics/node-can-bridge#30 can be tested/finalized/merged?

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.

5 participants