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 libtiff_internal from source #20331

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Oct 9, 2023

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

Towards #17231.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: newly deprecated This pull request contains new deprecations release notes: fix This pull request contains fixes (no new features) labels Oct 9, 2023
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-release please
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-release please
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-release please
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-release please

Deprecate libtiff from the host OS and remove it from setup prereqs.
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review October 9, 2023 22:11
@jwnimmer-tri
Copy link
Collaborator Author

+@zachfang for feature review of the TIFF configuration, please. (Specifically, I mean the settings of FOO_SUPPORT macros scattered throughout package.BUILD.bazel, e.g., for LZW vs CCITT vs JPEG etc.)

+@rpoyner-tri for feature review of the build system stuff, please -- and also for platform review per schedule (tomorrow).

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: for both requests to me.

Reviewed 13 of 15 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee zachfang

Copy link
Contributor

@zachfang zachfang left a comment

Choose a reason for hiding this comment

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

:lgtm:

From my understanding, the motivation to build internally is to carve out the files/dependencies we don't need so we have a smaller footprint?

Reviewed 13 of 15 files at r1, 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion


tools/workspace/libtiff_internal/repository.bzl line 9 at r2 (raw file):

        name = name,
        # N.B. Upstream is https://gitlab.com/libtiff/libtiff but this github
        # mirror seems to be kept up to date.

BTW. Is this because we prefer a GitHub source to Gitlab?

Code quote:

        # N.B. Upstream is https://gitlab.com/libtiff/libtiff but this github
        # mirror seems to be kept up to date.

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.

From my understanding, the motivation to build internally is to carve out the files/dependencies we don't need so we have a smaller footprint?

There are several reasons. That's one of them, but there are more also:

(2) Have the normal builds and wheel builds use the same tiff build system, to reduce the possibility that wheels are broken and we don't notice it quickly enough.

(3) Per #17231, ensure that we don't impose any particular version of libtiff on our users, in case they want a different one.

(4) When debugging, gdb will have line numbers & local variables & etc for libtiff in case we need to step into it. Ditto for ASan, etc.

(5) We can fix bugs in libtiff without waiting for anyone else to make new binaries for us.

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


tools/workspace/libtiff_internal/repository.bzl line 9 at r2 (raw file):

Previously, zachfang wrote…

BTW. Is this because we prefer a GitHub source to Gitlab?

Yes. We have repository rule sugar ("github_archive") for downloading code from github, keeping a mirror of it, and noticing when a new release has been posted so that we can upgrade. We don't have that infrastructure built up for gitlab.

@jwnimmer-tri jwnimmer-tri merged commit dfdc551 into RobotLocomotion:master Oct 10, 2023
1 check passed
@jwnimmer-tri jwnimmer-tri deleted the vtk-tiff branch October 10, 2023 18:43
@zachfang
Copy link
Contributor

Thanks for the clarifications!

WawasCode pushed a commit to WawasCode/drake that referenced this pull request Oct 31, 2023
Deprecate libtiff from the host OS and remove it from setup prereqs.
WawasCode pushed a commit to WawasCode/drake that referenced this pull request Oct 31, 2023
Deprecate libtiff 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: fix This pull request contains fixes (no new features) release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants