-
Notifications
You must be signed in to change notification settings - Fork 122
Add macOS ARM support for Thunderscope #3496
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
base: master
Are you sure you want to change the base?
Conversation
3ca31a1 to
c308ffb
Compare
environment_setup/util.sh
Outdated
| install_java_macos () { | ||
| java_home="" | ||
| java_download=https://download.oracle.com/java/21/latest/jdk-21_macos-aarch64_bin.tar.gz | ||
| curl -L $java_download -o /tmp/tbots_download_cache/jdk-21.tar.gz | ||
| mkdir /tmp/tbots_download_cache/jdk | ||
| tar -xzf /tmp/tbots_download_cache/jdk-21.tar.gz -C /tmp/tbots_download_cache | ||
| sudo mv /tmp/tbots_download_cache/jdk-21*/Contents/Home /opt/tbotspython/bin/jdk | ||
| rm /tmp/tbots_download_cache/jdk-21.tar.gz | ||
| rm -rf /tmp/tbots_download_cache/jdk | ||
| } | ||
|
|
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.
Java Installed already from brew, I don't think we need 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.
You need a specific version of Java for autoref (Java 21)
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 suppose you're using the autoref binary package, so you can probably ignore this
itsarune
left a comment
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.
Left a few comments
environment_setup/util.sh
Outdated
| install_java_macos () { | ||
| java_home="" | ||
| java_download=https://download.oracle.com/java/21/latest/jdk-21_macos-aarch64_bin.tar.gz | ||
| curl -L $java_download -o /tmp/tbots_download_cache/jdk-21.tar.gz | ||
| mkdir /tmp/tbots_download_cache/jdk | ||
| tar -xzf /tmp/tbots_download_cache/jdk-21.tar.gz -C /tmp/tbots_download_cache | ||
| sudo mv /tmp/tbots_download_cache/jdk-21*/Contents/Home /opt/tbotspython/bin/jdk | ||
| rm /tmp/tbots_download_cache/jdk-21.tar.gz | ||
| rm -rf /tmp/tbots_download_cache/jdk | ||
| } | ||
|
|
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 suppose you're using the autoref binary package, so you can probably ignore 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.
Consider using uname -s for "Darwin" and merge into the non-mac version of these functions to reduce clutter
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.
You should make sure you're using the same c++ compuler version as Ubuntu (to minimize the number of c++ compatibility changes that are needed)
| requirement("evdev"), | ||
| ], | ||
| ] + select({ | ||
| # TODO: remove this selection when we replace evdev to |
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.
Tag an issue with the TODO
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.
Can you use the target_compatible_with attribute?
https://bazel.build/reference/be/common-definitions#common.target_compatible_with
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 that was the first thing we have tried, but it breaks the dependency tree because other targets depending one this are all marked incompatible. I will do more test on this.
c308ffb to
d6b303d
Compare
|
|
||
| import evdev | ||
| from evdev import ecodes | ||
| # TODO: remove the try-catch when we rewrite this with macOS-compatible lib |
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.
Maybe we don't need this with this change: https://github.com/UBC-Thunderbots/Software/pull/3496/files#r2551713285
src/.bazelrc
Outdated
| # Escalate Warnings to fail Compile for Thunderbots code | ||
| build --features=external_include_paths | ||
| build --per_file_copt=proto/.*,proto/message_translation/.*,proto/primitive/.*,software/.*,shared/.*,-external/.*@-Wall,-Wextra,-Wno-unused-parameter,-Wno-deprecated,-Werror,-Wno-deprecated-declarations | ||
| build:linux --features=external_include_paths |
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.
Ideally these flags would also work on macos
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.
Yep, ideally it should also work on macos, but because the macos clang compiler is stricter than linux and reports millions of new errors (converted from warnings). :(
itsarune
left a comment
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.
Left some comments
I think we are getting close w/ getting this working, I put in a PR last week to hopefully reconcile some bazel hermiticity issues and we'll run a test this week to check if it actually works on anyone else's machines aside from minghao's. |
| qt@6 | ||
| node@20 | ||
| [email protected] | ||
| clang-format@20 |
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.
just a note, we did switch to clang-format-14 in #3499 which affects the formatting
|
Just a note, I did get this working on my m1 macbook as well. I did however have to resolve some c++ compatibility issues, and also update the package required for bazel refresh_compile_commands to resolve a bug. I'm not sure if these issues were also present on your macbook? We should definitely test some time with a clean install, and to test all scripts and commands that we want to support for mac to make sure they also work. |
|
@nycrat Personally, I dont use the refresh_compile_commands. That's a great catch. I think it is less likely to affect ubuntu users by just updating the version. I will cherry-pick your commit onto this. |
closes #3529
Description
This PR introduces native macOS ARM support for Thunderscope.
Currently, this PR is experimental and is just providing a minimal working demo on my macbook M1.
Out of scope for this PR:
Testing Done
TODO List
For this PR:
local_new_repositoriesand confirm compatibility with Ubuntuevdevdoes not exist for macos.Maybe for future PRs or possibly this PR:
TbotsProto.ObstacleListto prevent system network buffer overload (TrajectoryPlanner improvements #3104). This is impacting the stability of thunderscope on macOS. I am currently working on a sliding window cache that hopefully can mitigate this issue.