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

[workspace] Build libpng_internal from source #20276

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Sep 28, 2023

Deprecate libpng from the host OS and remove it from setup prereqs.

Towards #17231 in wheel builds.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review September 28, 2023 22:16
Copy link
Contributor

@calderpg-tri calderpg-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers


tools/workspace/libpng_internal/package.BUILD.bazel line 93 at r1 (raw file):

            # Use only the SSE2 instructions (not SSE3, AVX2, etc.)
            # This matches the Debian default (i.e., the psABI baseline).
            "-DPNG_INTEL_SSE_IMPLEMENTATION=1",

fwiw I think we would be fine having the baseline be SSE4 + POPCNT. That would cover everything released in the last decade from Intel (Sandy Bridge+ (2010), Silvermont+ (2013)) and AMD (Bulldozer+ (2011), Jaguar+ (2013)).

Copy link
Collaborator Author

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

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers


tools/workspace/libpng_internal/package.BUILD.bazel line 93 at r1 (raw file):

Previously, calderpg-tri wrote…

fwiw I think we would be fine having the baseline be SSE4 + POPCNT. That would cover everything released in the last decade from Intel (Sandy Bridge+ (2010), Silvermont+ (2013)) and AMD (Bulldozer+ (2011), Jaguar+ (2013)).

Good point. Done.

@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please.

Copy link
Contributor

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

:lgtm:

Reviewed 12 of 15 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: needs at least two assigned reviewers

@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for platform review per schedule, please.

Deprecate libpng from the host OS and remove it from setup prereqs.
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-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: WIth a question for my own edification about deprecation vs distribution.

Reviewed 12 of 15 files at r1, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion


setup/ubuntu/binary_distribution/packages-focal.txt line 25 at r4 (raw file):

libogg0
libopengl0
libpng16-16

In both of these distribution, what are the odds that the user got libpng because of drake? Admittedly, they'll still have it until they do an install on a new machine. Presumably, we don't make promises about our dependencies being stable (although, we did make the effort vis a vis VTK).

From source, it's not a problem. If they reference the deprecated target, they'll be warned, but it will continue to pull the dependency and use it, right? It's only people who use drake from install that would have the chance to trip over this?

Copy link
Collaborator Author

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

Reviewable status: 1 unresolved discussion


setup/ubuntu/binary_distribution/packages-focal.txt line 25 at r4 (raw file):

... what are the odds that the user got libpng because of drake? ..

There's no way to know.

... we don't make promises about our dependencies being stable ...

Correct. The https://drake.mit.edu/stable.html only promises that OS things we require will be installed by install_prereqs. It doesn't mention how/when we remove things. (Maybe it should explicitly disclaim stability for the package lists?)

...although, we did make the effort vis a vis VTK ...

I don't believe we did. The commit message for #19945 says:

The repository rules will continue to work during the deprecation
window, but Drake no longer installs their Ubuntu packages; if you
need the packages, ensure your project setup scripts install them.

From source, it's not a problem. If they reference the deprecated target, they'll be warned, but it will continue to pull the dependency and use it, right? It's only people who use drake from install that would have the chance to trip over this?

I wouldn't phrase it that way. Instead, I'd say the cases which might cause breakage are when the user was invoking Drake's install_prereqs as a subroutine (whether they are doing a source build or using our binaries), and assuming that it does anything more than prepare the system to build (or just run) Drake. That's one reason why drake-external-examples exhibits vendored copies of Drake's package lists, and why Anzu likewise has vendored copies. A smart user should be inspecting and approving/rejecting any changes to those lists.

Copy link
Contributor

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)


setup/ubuntu/binary_distribution/packages-focal.txt line 25 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

... what are the odds that the user got libpng because of drake? ..

There's no way to know.

... we don't make promises about our dependencies being stable ...

Correct. The https://drake.mit.edu/stable.html only promises that OS things we require will be installed by install_prereqs. It doesn't mention how/when we remove things. (Maybe it should explicitly disclaim stability for the package lists?)

...although, we did make the effort vis a vis VTK ...

I don't believe we did. The commit message for #19945 says:

The repository rules will continue to work during the deprecation
window, but Drake no longer installs their Ubuntu packages; if you
need the packages, ensure your project setup scripts install them.

From source, it's not a problem. If they reference the deprecated target, they'll be warned, but it will continue to pull the dependency and use it, right? It's only people who use drake from install that would have the chance to trip over this?

I wouldn't phrase it that way. Instead, I'd say the cases which might cause breakage are when the user was invoking Drake's install_prereqs as a subroutine (whether they are doing a source build or using our binaries), and assuming that it does anything more than prepare the system to build (or just run) Drake. That's one reason why drake-external-examples exhibits vendored copies of Drake's package lists, and why Anzu likewise has vendored copies. A smart user should be inspecting and approving/rejecting any changes to those lists.

Thanks for the clarification.

(Sorry, I hadn't meant to make that blocking.)

@SeanCurtis-TRI SeanCurtis-TRI merged commit 7e8245c into RobotLocomotion:master Oct 2, 2023
10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the vtk-png branch October 2, 2023 20:30
WawasCode pushed a commit to WawasCode/drake that referenced this pull request Oct 31, 2023
Deprecate libpng from the host OS and remove it from setup prereqs.
@jwnimmer-tri jwnimmer-tri mentioned this pull request Mar 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: feature This pull request contains a new feature release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants