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 tinyxml2 from source #17830

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Sep 3, 2022

Deprecate the repository rule for the OS library.

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 Sep 3, 2022
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review September 3, 2022 15:52
Deprecate the repository rule for the OS library.
@jwnimmer-tri jwnimmer-tri force-pushed the workspace-tinyxml-internal branch from 335799d to 9585b5e Compare September 5, 2022 14:55
@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 22 of 24 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for platform review tomorrow, please.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

platform :lgtm: with a question.

Reviewed 22 of 24 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


tools/workspace/default.bzl line 355 at r2 (raw file):

    if "tinyobjloader" not in excludes:
        tinyobjloader_repository(name = "tinyobjloader", mirrors = mirrors)
    if "tinyxml2" not in excludes:

question: do we still need 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 (waiting on @xuchenhan-tri)


tools/workspace/default.bzl line 355 at r2 (raw file):

Previously, xuchenhan-tri wrote…

question: do we still need this?

Yes, per the Stable API section about default.bzl "... we will deprecate it prior to removing our definition of the dependency".

Copy link
Contributor

@xuchenhan-tri xuchenhan-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),xuchenhan-tri(platform) (waiting on @jwnimmer-tri)


tools/workspace/default.bzl line 355 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, per the Stable API section about default.bzl "... we will deprecate it prior to removing our definition of the dependency".

I see. Thanks for the reference.

@xuchenhan-tri xuchenhan-tri merged commit 78eaea8 into RobotLocomotion:master Sep 6, 2022
@jwnimmer-tri jwnimmer-tri deleted the workspace-tinyxml-internal branch September 6, 2022 17:59
@jwnimmer-tri jwnimmer-tri mentioned this pull request Feb 2, 2023
10 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