-
Notifications
You must be signed in to change notification settings - Fork 350
Add build support for Ubuntu 24.04 #4186
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
Conversation
a32ed49 to
8d57714
Compare
|
@adamkewley FYI I'm working towards officially supporting Ubuntu 24 (and Windows 2025, while I'm at it) in the CI. I've updated |
|
Not sure if upgrading to swig 4.4 can help, worth trying. If we can upgrade swig instead of changing code that's likely less error prone, eventually we maybe able to retire the swigsimtk hack and use the headers from simbody as is. Wishful thinking... |
Why is this hack needed again? |
Because swig can't digest simbody headers and templates as is. |
93460ce to
c5acdbd
Compare
|
@aymanhab, as discussed, I now have working builds for Ubuntu 24.04 on CI. Here is a summary of changes that I've made on this branch:
You've also made several changes to the CI for supporting PyPI builds. I'm not sure what changes should be merged first. Perhaps we get the PyPI builds working and then we PR in the above changes piecemeal. Let me know what you think makes the most sense. |
|
@nickbianco Lets get your PR in first so that we can make sure pypi builds work with the more recent compilers/platforms |
2ebbed1 to
e4ddd3d
Compare
e4ddd3d to
d26be79
Compare
|
@aymanhab I've incorporated a few last changes and cleaned up the git history, so this is now ready for review. |
aymanhab
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.
LGTM, minor questions to clarify. Thank you
@aymanhab reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nickbianco)
.github/workflows/continuous_integration.yml line 64 at r1 (raw file):
- name: Build dependencies # if: steps.cache-dependencies.outputs.cache-hit != 'true' run: |
With frequent builds to test ci changes, I'd uncomment this line whenever possible
.github/workflows/continuous_integration.yml line 420 at r1 (raw file):
- name: Build dependencies # if: steps.cache-dependencies.outputs.cache-hit != 'true' run: |
Same with note above about dependencies build/cache
.github/workflows/continuous_integration.yml line 428 at r1 (raw file):
DEP_CMAKE_ARGS+=(-DSUPERBUILD_ezc3d=ON) DEP_CMAKE_ARGS+=(-DOPENSIM_WITH_CASADI=ON) printf '%s\n' "${DEP_CMAKE_ARGS[@]}"
I remember DISABLE_LOG_FILE was necessary otherwise a random empty log file was generated. Any reason for removal now?
dependencies/CMakeLists.txt line 189 at r1 (raw file):
GIT_URL https://github.com/simbody/simbody.git GIT_TAG f9ab12cbad9d0da106473259d34c50577f934f49 CMAKE_ARGS -DBUILD_EXAMPLES:BOOL=OFF
I'm assuming the library names on linux lib*so.3.9 have not changed due to this Simbody commit upgrade
nickbianco
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.
Thanks @aymanhab, ready for review again.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @aymanhab)
.github/workflows/continuous_integration.yml line 64 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
With frequent builds to test ci changes, I'd uncomment this line whenever possible
Done.
.github/workflows/continuous_integration.yml line 420 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Same with note above about dependencies build/cache
Done.
.github/workflows/continuous_integration.yml line 428 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
I remember DISABLE_LOG_FILE was necessary otherwise a random empty log file was generated. Any reason for removal now?
This flag was not actually set for the Ubuntu builds previously. The diff got mangled, so it looks like it did. But no harm in adding it here anyway.
dependencies/CMakeLists.txt line 189 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
I'm assuming the library names on linux lib*so.3.9 have not changed due to this Simbody commit upgrade
I don't believe so, and I assume things would break if they did?
aymanhab
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.
@aymanhab reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)
dependencies/CMakeLists.txt line 189 at r1 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
I don't believe so, and I assume things would break if they did?
Assuming the caching is working properly you're right.
Summary of changes, some of which are "drive-by" improvements to the CI workflow: - Upgraded Python version to 3.11 (with NumPy 2.0) on CI - Removed "perf" tests on CI - Added Ubuntu 24.04 CI job - Updated Windows CI job to use windows-2025 runner - Updated the Simbody commit and Mat.h and Vec.h under Bindings/SWIGSimTK based on recent Simbody changes. This change is necessary to avoid a warning that newer compilers (e.g., GCC 13) about potential out-of-bounds errors in SimTK::Mat constructors. - Converted references to copies in testOutputReporter.cpp to avoid dangling reference errors from GCC - Changed how data members in WrapResult and PathWrapPoint are initialized to avoid "possibly uninitialized" errors from GCC
de9143e to
27acd24
Compare
|
@aymanhab I checked the Simbody commits and confirmed that the library names have not changed. I will merge after the latest CI run passes. |
|
Merging. Thanks @aymanhab! @adamkewley you are now set to unleash your full arsenal of STL wizardry in OpenSim. |
|
Glad to hear it 😉 @nickbianco / @aymanhab - I should have some time to start PRing refactors soon-ish The only thing I'm unsure about is using C++20 library symbols that aren't in Ubuntu22 yet, unless the consensus is that OpenSim >4.6 will be shipped against Ubuntu24? In that case, I'd likely have to push OSC up to Ubuntu 24 anyway, given it uses OpenSim extensively. Ubuntu 22's free-tier support ends May 2027. I would guess that robot labs etc. would leave their upgrade as late as possible (probably after that window, even). Ubuntu 22 supports a newer gcc compiler, so C++20 (and even C++23) language features are available in Ubuntu 22, but library support is still stuck with Ubuntu 22's C++ But it means that (e.g.) The alternative--which we should definitely investigate--is something like compiling the project in CentOS/AlmaLinux with |
|
The vast majority of our users in the biomechanics community use Windows and Mac, and we haven't actually officially distribute Linux releases of the main (GUI) application. The only official Linux "release" we have are the conda packages; however, the Linux packages for the 4.5.2 release do seem to be the most popular. So maybe there is an argument to start "officially" releasing OpenSim, starting with Ubuntu 24? I'm not totally sure, but having to wait for
So this would effectively replace our Ubuntu builds but would allow users to install on more Linux distros? That would definitely be nice. |
|
Thanks @nickbianco and @adamkewley for looking into these upgrades. To make 4.6 manageable, I'd request we hold off on major changes until we branch 4.6 in the next couple of days. |
I don't know the demographics, but I'd guess (purely, a guess) the Mac/Windows users are more likely than Linux users to use MATLAB when scripting. Behaviorally (another guess), the Linux users would also be more likely to do stuff like automating their virtual environment creation with a bash script, run code via docker, etc., so it makes sense that the download counts for the python bindings skew towards Linux. As additional information, OSC's downloads are roughly 11 % Linux, 21 % MacOS, and 68 % Windows. The concern about picking an OS version is that if you have Ubuntu 22/20 users using the 4.5.2 bindings then upgrading the required OS will cause them to either version-freeze on the earlier OpenSim (fine, if unfortunate) or force them to upgrade their whole stack to Ubuntu 24 (pure software groups: maybe fine, hardware groups: will delay the upgrade as long as possible because OS upgrades with ROS etc. could potentially brick their stack). Using an older OS with backported C++ compilers (usually, CentOS) can be a useful trick in these situations. It can yield binaries that only depend on old libstdc++ symbols. If you're building a library that doesn't have additional system dependencies, you should be able to repackage that binary for Ubuntu, etc. I can already can confirm from my own work on OSC that Simbody + OpenSim + OpenBLAS can be statically compiled into a single binary that's only dependent on glibc + libstdc++ (ideal), but I don't know whether that remains the case when Moco's runtime dependency tree is added into the mix - it'd require investigation (noting, Moco uses casadi, which is lGPL-licensed). |
|
I would guess the same about the demographics of our Windows/Mac versus Linux users.
Ah okay, interesting. That is something I'm not familiar with, but I agree it seems worth considering. Perhaps we could discuss some options after upcoming the OpenSim 4.6 release. |
Fixes issue #4184
Brief summary of changes
Some of these changes are drive-by improvements, mostly to the CI system.
windows-2025runner.Mat.handVec.hunderBindings/SWIGSimTKbased on @adamkewley's recent Simbody changes. This change is necessary to avoid a warning that newer compilers (e.g., GCC 13) about potential out-of-bounds errors inSimTK::Matconstructors.testOutputReporter.cppto avoid dangling reference errors from GCC.WrapResultandPathWrapPointare initialized to avoid "possibly uninitialized" errors from GCC.Testing I've completed
Ran tests locally and on CI.
Looking for feedback on...
CHANGELOG.md (choose one)
This change is