-
Notifications
You must be signed in to change notification settings - Fork 432
Upgrade CI to newer os version #635
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?
Upgrade CI to newer os version #635
Conversation
6a0db5e
to
0c9ca3c
Compare
xref: #626 |
macos-11 is no longer supported by github. ubuntu 20 is likewise ridiculously old. This includes benign changes to make the mac compiler happy.
0c9ca3c
to
a1f3377
Compare
Thanks! I'm sorry I missed your earlier PR. |
bc1d3e4
to
0d7c964
Compare
0d7c964
to
9a80497
Compare
798e2cc
to
4563a74
Compare
4563a74
to
b8e1e60
Compare
I've exhausted my best efforts at making the mac build happy. Unless someone else can contribute more than I can, I'm going to replace this PR with one that simply no longer tries to test against mac. |
Which configuration are interested in testing? Downstream we are already packaging fcl on mac in conda-forge (https://github.com/conda-forge/fcl-feedstock), so it is trivial for us to contribute a mac ci based on conda-forge dependencies as opposed to brew one, see traversaro#1 for an example. In there the compilation works fine, while there are three tests that fail on macOS (from the log at https://github.com/traversaro/fcl/actions/runs/13830174941/job/38692485376?pr=1):
all tests are passing fine on macOS. No problem if you not interesting in having this in the CI as actually you are interested in mac with dependencies provided by brew, not dependencies provided by another package manager. |
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.
In this case, the failing dependencies aren't coming through brew, they're coming from the CI pre-building a specific version of octomap.
However, I'm incredibly mac ignorant and would welcome any help you might be able to offer. :)
Reviewable status: 0 of 4 files reviewed, all discussions resolved
I think octomap was built from source as back in february 2018 homebrew science (that contained octomap) was deprecated, and octomap was not migrated to core, see #262 . However, in March 2018 octomap was added back in homebrew (see Homebrew/homebrew-core#24528) and it is still available there: https://formulae.brew.sh/formula/octomap . I may be missing something, but there is any reason for which we can't simply brew install octomap instead of compiling from source in https://github.com/flexible-collision-library/fcl/blob/master/ci/install_osx.sh#L8-L16 ? |
Currently, fcl is pinned to an older version of octomap. This is generally bad because it also limits this CI workflow working on newer Linux CI as well (the pinned octomap version simply isn't ready for modern compilers). So, generally, it would be good to modernize the octomap. I haven't looked into what that would take. I don't know how things have changed in the 9 years between the pinned version and the current version. But, evaluating that is probably the first step. |
I did a quick check in https://github.com/flexible-collision-library/fcl/actions/runs/13901821176/job/38895257356, and it seems that compilation wise everything works fine, while for testing 4 tests are failing:
however, at the first glance the failures are not octomap related. |
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.
That's good to know. Can you do me a favor and push to this PR the change to modernize octomap and I can look at the tests.
Reviewable status: 0 of 4 files reviewed, all discussions resolved
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.
The test failures are somewhat surprising; they shouldn't have anything to do with octomap....hmmm....
Reviewable status: 0 of 4 files reviewed, all discussions resolved
I am probably missing something, but what do you mean with "push to this PR"? I can't push on your fork. |
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 can. :) You have several options:
- Use the github API to edit the branch directly (tedious for all but the simplest of changes).
- Check out my actual branch, add a commit, and push it back to my repo.
- Alternatively, if you share with me your branch, I'll just cherry pick the changes necessary to reproduce the problem.
I'm good with any of the above.
Reviewable status: 0 of 4 files reviewed, all discussions resolved
Sorry, I feel really I am missing something.
pushing to you repo give me 403, as I expected.
The branch is the one used in #636, the specific commit ed65720 . |
macos-11 is no longer supported by github.
ubuntu 20 is likewise ridiculously old.
This includes benign changes to make the mac compiler happy.
This change is