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

Remove unused libyaml and libdl2 dependencies #18734

Merged

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Feb 2, 2023

Toward #17161.

This change is Reviewable

@mwoehlke-kitware
Copy link
Contributor Author

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-everything-release please
@drake-jenkins-bot linux-jammy-unprovisioned-clang-bazel-experimental-everything-release please
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-everything-release please
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-cmake-experimental-everything-release please

+@svenevs for feature review, please. (Does this seem like sufficient coverage for pre-merge?)

@jwnimmer-tri
Copy link
Collaborator

There is no direct CI coverage of the binary_distribution text files, since the source_distribution usually trumps them anyway and we never install "binary" on their own (since by its nature, Drake CI is always building from source). The closest we have to CI coverage are the drake-external-examples jobs, but that code currently forks the package lists into its git tree, instead of using the list embedded in the packages it downloads, so you'd need to do weird hijinks there to get it testing what you want it to test.

That's all a bit of a hassle.

To me, testing locally manually starting from an empty Docker image would be easiest:

  • Boot an empty Docker (once on Focal, and once on Jammy).
  • In that container, run the version of binary_distribution/install_prereqs.sh from this PR. Notably the binary script, not the source one.
  • Unpack last night's nightly tgz release (https://drake.mit.edu/from_binary.html) into the container.
  • Try import pydrake.all. If there are missing shared libraries, it will complain.

@mwoehlke-kitware
Copy link
Contributor Author

... testing locally manually ...

Done. No import errors.

@svenevs svenevs mentioned this pull request Feb 7, 2023
10 tasks
@svenevs
Copy link
Contributor

svenevs commented Feb 7, 2023

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-snopt-mosek-packaging please.
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-snopt-mosek-packaging please.

These jobs are additionally needed to evaluate the changes here, the contents of setup/ubuntu/packages-* in this PR change the final packaged dependencies.

Copy link
Contributor

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

:lgtm: +@jwnimmer-tri for platform review, per schedule, please.

To me, testing locally manually starting from an empty Docker image would be easiest:

  • Boot an empty Docker (once on Focal, and once on Jammy).
  • In that container, run the version of binary_distribution/install_prereqs.sh from this PR. Notably the binary script, not the source one.
  • Unpack last night's nightly tgz release (https://drake.mit.edu/from_binary.html) into the container.
  • Try import 1pydrake.all`. If there are missing shared libraries, it will complain.

Feature review repeated these steps and they were successful. I also confirmed from a fresh macOS brew state that libyaml can be removed (this was never tracked anywhere, it is OK for this PR to remove it).

@mwoehlke-kitware I added more information on how to do this package analysis as you finish off the ticket in #17161 (comment)

@jwnimmer-tri there are some new potential issues from this analysis, but none relate to this PR. shlibs:Depends vs our packages here, removing libdl2 and libyaml-cpp-dev is A-OK.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), missing label for release notes (waiting on @jwnimmer-tri and @mwoehlke-kitware)


-- commits line 2 at r1:
nit: on squash perhaps state them explicitly (libyaml and libdl)? Remove unused libyaml and libdl dependencies?

Copy link
Collaborator

@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: platform.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, missing label for release notes (waiting on @mwoehlke-kitware)

Copy link
Contributor

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: missing label for release notes (waiting on @mwoehlke-kitware)


-- commits line 2 at r1:

Previously, svenevs (Stephen McDowell) wrote…

nit: on squash perhaps state them explicitly (libyaml and libdl)? Remove unused libyaml and libdl dependencies?

Done.

@svenevs svenevs changed the title Don't install no longer needed dependencies Remove unused libyaml and libdl2 dependencies Feb 8, 2023
@svenevs svenevs added the release notes: fix This pull request contains fixes (no new features) label Feb 8, 2023
@svenevs svenevs merged commit 03d3ca1 into RobotLocomotion:master Feb 8, 2023
@mwoehlke-kitware mwoehlke-kitware deleted the cleanup-old-dependencies branch February 8, 2023 20:19
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants