Skip to content

Add a build script for wx_armor - #25

Merged
QuantuMope merged 3 commits into
mainfrom
PR/andrew/build_script
Sep 5, 2025
Merged

Add a build script for wx_armor#25
QuantuMope merged 3 commits into
mainfrom
PR/andrew/build_script

Conversation

@QuantuMope

Copy link
Copy Markdown
Contributor

This PR adds an automated build script for wx_armor. It's quite annoying / tedious to have to install it on fresh machines. This script should now handle all necessary dependencies seamlessly.

Tested on two separate machines.

An old tutorial can be found here for reference.
https://horizonrobotics.feishu.cn/wiki/NYL6wgBc8i05O0kLkk2cqFNxn2g

@le-horizon le-horizon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the nice PR. A couple suggestions if they make sense.

Comment thread README.md Outdated

Users must also set `LD_LIBRARY_PATH` to the shared library locations. In your `~/.bashrc`, add the following line:
```bash
export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add a check in build.sh to add this to .bashrc if not already there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried doing this, but since this build script is being run using sudo, it uses the root's .bashrc instead. Since there's no easy fix for this, I simply omitted it. Instead, I have the part about setting LD_LIBRARY_PATH be the first step in README.

Comment thread README.md
- `sudo apt install -y yaml-cpp-dev`

#### Building From Source

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add some explanation about the system requirements, e.g. ubuntu version tested etc..
Also add a link to our other repo for running on Jetson devices.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

@QuantuMope QuantuMope left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey Le, thanks for the review. Addressed your comments. PTAL.

I can also confirm that the build script works flawlessly on Ubuntu 24.04, in addition to 22.04.

Comment thread README.md
- `sudo apt install -y yaml-cpp-dev`

#### Building From Source

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

Comment thread README.md Outdated

Users must also set `LD_LIBRARY_PATH` to the shared library locations. In your `~/.bashrc`, add the following line:
```bash
export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried doing this, but since this build script is being run using sudo, it uses the root's .bashrc instead. Since there's no easy fix for this, I simply omitted it. Instead, I have the part about setting LD_LIBRARY_PATH be the first step in README.

le-horizon
le-horizon previously approved these changes Dec 27, 2024

@le-horizon le-horizon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LG. Btw, is there any instruction about udev rules? wx_armor fails to talk to the arm w/o the udev rules to define /dev/ttyDXL

@QuantuMope
QuantuMope force-pushed the PR/andrew/build_script branch from 7e5238d to 89a076f Compare August 25, 2025 19:40
@QuantuMope

Copy link
Copy Markdown
Contributor Author

Forced pushed a rebase to main @le-horizon

@le-horizon le-horizon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the automation. Some suggestions to make the process even more automatic.

Comment thread build.sh Outdated
Comment thread build.sh Outdated
Comment on lines +64 to +76
# Install json
cd /tmp
rm -rf json
git clone https://github.com/nlohmann/json
cd json
mkdir build
cd build
cmake ..
make -j"$(nproc)"
make install
cd /tmp
rm -rf json
cd "$HOME_DIR"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will do:
sudo apt-get install -y nlohmann-json3-dev

Also need these:
sudo apt-get install -y udev libyaml-cpp-dev libspdlog-dev gcc g++ cmake libjsoncpp-dev uuid-dev zlib1g-dev

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread build.sh Outdated
Comment thread build.sh
@le-horizon

Copy link
Copy Markdown
Contributor

@QuantuMope could you work on this PR so we can close #33?

Also need to avoid using export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH, but instead improve the build script. This env var is perhaps one of the last few things toward a fully automated install.

@QuantuMope
QuantuMope force-pushed the PR/andrew/build_script branch 2 times, most recently from f1cfc45 to 5cddb5e Compare September 5, 2025 18:14

@QuantuMope QuantuMope left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey Le, addressed all comments. Also, added a cmake fix to avoid the .bashrc LD_LIBRARY_PATH setting.

Tested that everything works on gail7.

Comment thread build.sh Outdated
Comment on lines +64 to +76
# Install json
cd /tmp
rm -rf json
git clone https://github.com/nlohmann/json
cd json
mkdir build
cd build
cmake ..
make -j"$(nproc)"
make install
cd /tmp
rm -rf json
cd "$HOME_DIR"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread build.sh
@QuantuMope
QuantuMope force-pushed the PR/andrew/build_script branch from b59ae20 to 22fcc66 Compare September 5, 2025 18:18

@le-horizon le-horizon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just one question. Thanks for the automation!

Comment thread wx_armor/CMakeLists.txt
spdlog::spdlog
nlohmann_json::nlohmann_json
yaml-cpp)
yaml-cpp::yaml-cpp)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dynamixel_sdk dependency seems to be missing, have you tested CMake w/o LD_LIBRARY_PATH?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I have. I unset it. I don't think that dependency existed before in the cmake. Could be that toolbox refers to sdk in its own cmake.

@QuantuMope
QuantuMope merged commit 515a8d1 into main Sep 5, 2025
@QuantuMope
QuantuMope deleted the PR/andrew/build_script branch September 5, 2025 19:09
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.

2 participants