Skip to content
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

Upgrade to Drake 1.28 #340

Merged
merged 16 commits into from
Apr 16, 2024
Merged

Upgrade to Drake 1.28 #340

merged 16 commits into from
Apr 16, 2024

Conversation

adityapande-1995
Copy link
Collaborator

@adityapande-1995 adityapande-1995 commented Mar 19, 2024

Upgrades drake version.


This change is Reviewable

Signed-off-by: Aditya Pande <adityapande@google.com>
Signed-off-by: Aditya Pande <adityapande@google.com>
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

@adityapande-1995
Copy link
Collaborator Author

I see a weird error locally :

adityapande@dr7:~/drake_stuff/drake-ros/drake_ros$ bazel build //...
INFO: Analyzed 67 targets (0 packages loaded, 0 targets configured).
INFO: Found 67 targets...
ERROR: /usr/local/google/home/adityapande/.cache/bazel/_bazel_adityapande/cd3fbbbc98b10f1c4b8f704b2957ac2b/external/drake/common/BUILD.bazel:307:17: Compiling common/find_resource.cc failed: (Exit 1): gcc failed: error executing command (from target @drake//common:_find_resource_compiled_cc_impl) /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++14' -MD -MF ... (remaining 61 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/drake/common/find_resource.cc: In function 'drake::Result drake::FindResource(const string&)':
external/drake/common/find_resource.cc:247:22: error: 'const string' {aka 'const class std::__cxx11::basic_string<char>'} has no member named 'starts_with'
  247 |   if (!resource_path.starts_with(prefix)) {
      |                      ^~~~~~~~~~~
INFO: Elapsed time: 4.570s, Critical Path: 3.96s
INFO: 46 processes: 41 internal, 5 linux-sandbox.
FAILED: Build did NOT complete successfully

@jwnimmer-tri
Copy link
Contributor

You probably need to update your C++ standard:

C++ standard >= 20

-- https://drake.mit.edu/release_notes/v1.27.0.html

Signed-off-by: Aditya Pande <adityapande@google.com>
@jwnimmer-tri
Copy link
Contributor

Sorry, I was just heading offline and didn't have a chance for a full reply. The file that owns the C++ standard is default.bazelrc in the root of this repo.

Signed-off-by: Aditya Pande <adityapande@google.com>
@jwnimmer-tri jwnimmer-tri self-assigned this Mar 21, 2024
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @adityapande-1995 and @IanTheEngineer)


default.bazelrc line 1 at r3 (raw file):

# Use C++17.

nit Comment is stale (should say 20)


.github/ci.bazelrc line 18 at r3 (raw file):

fetch --disk_cache /home/runner/.cache/bazel_ci/bazel_local_disk
build --disk_cache /home/runner/.cache/bazel_ci/bazel_local_disk
build --cxxopt="-std=c++20"

nit Remove

Signed-off-by: Aditya Pande <adityapande@google.com>
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

@adityapande-1995
Copy link
Collaborator Author

Not sure what the CI failure is about, I'm not seeing it locally

@jwnimmer-tri
Copy link
Contributor

I don't understand the failure shown in attempt 1, but for the error on attempt 2 (documentation_pybind.h), it looks like something installed a broken version of GCC 13 that is tripping up Drake's clang-based header file scraper. I expect the same break would happen if you re-ran the master branch CI. Specifically, looking at the full build logs, it looks like someone is installing the https://ppa.launchpadcontent.net/ubuntu-toolchain-r/test/ubuntu PPA. That's a likely culprit for the broken GCC install.

@jwnimmer-tri
Copy link
Contributor

Yes per readme the Ubuntu PPA is added in the default GHA Ubuntu image.

This is somewhat related to actions/runner-images#8659.

This error probably does not happen on master, since it's due to the C++20 upgrade. Drake is using clang-14 to parse the header files when making bindings, but the newer GCC 13 libstdc++ is not compatible with that version of Clang.

The work-around is probably to apt purge the GCC 13 (and maybe GCC 12) stuff that we don't need, as part of the CI setup scripts. The normal GCC 11 (default version on Ubuntu 22) is fine by itself.

You could image doing two different PRs here -- one to bump to C++20 (and fix GCC) but keep Drake at v1.26. Then cycle back here to upgrade 1.26 => 1.27 which should be just the tiny version bump with no other changes.

@adityapande-1995
Copy link
Collaborator Author

Makes sense. Moving to cpp 20 changes here : #341

@adityapande-1995 adityapande-1995 marked this pull request as draft March 25, 2024 18:33
@adityapande-1995 adityapande-1995 marked this pull request as ready for review March 25, 2024 20:01
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Seems like the error is still there. Looks our github actions setup here will need to purge GCC 13 (and maybe 12) to get this passing.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

Signed-off-by: Aditya Pande <adityapande@google.com>
Signed-off-by: Aditya Pande <adityapande@google.com>
@jwnimmer-tri
Copy link
Contributor

I think libstdc++-13-dev also needs to be removed. You might try a dpkg -l '*-13*' as a debugging aid to find all 13-infected package names.

@jwnimmer-tri jwnimmer-tri changed the title Upgrade to drake 1.27 Upgrade to Drake 1.28 Apr 12, 2024
@jwnimmer-tri
Copy link
Contributor

I think the GitHub Actions stuff is solved now. There are two more problems -- new linter errors from the upgraded buildifier, and new test failures due to using unstable model files from Drake. I'll try to push fixes for both of those soon.

@jwnimmer-tri
Copy link
Contributor

+@rpoyner-tri could you please take a look at the CI error here? (https://github.com/RobotLocomotion/drake-ros/actions/runs/8698347467/job/23855186943?pr=340#step:22:46)

It looks to me like the parser.SetAutoRenaming(true) is not working as intended for collision filter groups. This might be a recent regression.

The questions are:

(1) Is there a Drake bug report we should file about this? (If so, can you please write it up.)

(2) Is there any quick work-around we could band-aid here, to get this PR merged?

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r7, 16 of 16 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

Copy link

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

(1) yeah kinda (the code presented would need the parser to get updated when RenameModelInstance happens on the plant; there's a design gap here. I will write an issue), (2) we can just move the construction of the Parser object inside the loops in the application code, as a workaround.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

@jwnimmer-tri
Copy link
Contributor

Ah, good point. I'll try to get (2) fixed here.

Copy link

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

I'm trying my proposed (2) in drake_ros, and it's still blowing up. I pasted a very similar swath of code into a (temporary) test in drake, and (2) fixed it. Still investigating.

Reviewable status: 18 of 20 files reviewed, all discussions resolved (waiting on @IanTheEngineer and @jwnimmer-tri)

Copy link

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Ah. multirobot.py also needs an analogous fix.

Reviewable status: 18 of 20 files reviewed, all discussions resolved (waiting on @IanTheEngineer and @jwnimmer-tri)

@jwnimmer-tri
Copy link
Contributor

So possibly the patch I just pushed here will be OK? I guess we'll find out soon.

Copy link

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

yup. looks good :lgtm:

Reviewable status: 18 of 20 files reviewed, all discussions resolved (waiting on @IanTheEngineer and @jwnimmer-tri)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @IanTheEngineer)

@jwnimmer-tri jwnimmer-tri merged commit d3f087c into main Apr 16, 2024
6 checks passed
@jwnimmer-tri jwnimmer-tri deleted the aditya/upgrade_drake_1.27 branch April 16, 2024 19:48
Copy link
Member

@IanTheEngineer IanTheEngineer left a comment

Choose a reason for hiding this comment

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

:LGTM: Thanks for iterating here, @jwnimmer-tri , and reviewing @rpoyner-tri

Reviewed 2 of 2 files at r7, 14 of 16 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@jwnimmer-tri
Copy link
Contributor

FYI actions/runner-images#9679 is underway, which means that the GCC 13 stuff will vanish on its own eventually, and we can remove our GHA CI hacks.

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.

None yet

5 participants