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 sdformat to 10.3.0 #14705

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Feb 25, 2021

Deprecate @tinyxml external
Ignore new <heightmap> elements
models: Fix now-invalid usages of /sdf//pose

Derived from / towards #14401

\cc @azeey


This change is Reviewable

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@SeanCurtis-TRI for feature review, please!

(Let me know if you'd be OK w/ serving as platform too)

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

@SeanCurtis-TRI SeanCurtis-TRI added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Feb 25, 2021
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.

Got some concerns about throwing.

Also, the commit message/PR description really weren't sufficient for me to understand the context of this.Presumably, 10.2.0 introduced height maps and changed the white space character set, yes? Is that the only relevant change? Because CI isn't happy. Generally, I'd request a bit more information so that it won't be quite the black box in the history.

+(status: single reviewer ok)

Reviewed 4 of 4 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @EricCousineau-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

      }
    }
    case sdf::GeometryType::HEIGHTMAP: {

nit: We don't routinely throw for unsupported SDF features. Or have I missed that?


tools/workspace/sdformat/package.BUILD.bazel, line 102 at r1 (raw file):

    "include/sdf/Geometry.hh",
    "include/sdf/Gui.hh",
    "include/sdf/Heightmap.hh",

This is obviously my ignorance, but we have to include headers for portions we don't support? Presumably, this represents the minimum dependency graph that we require to build what we actually want, yes?

@jwnimmer-tri
Copy link
Collaborator


tools/workspace/sdformat/package.BUILD.bazel, line 102 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This is obviously my ignorance, but we have to include headers for portions we don't support? Presumably, this represents the minimum dependency graph that we require to build what we actually want, yes?

There's a comment (behind the fold in reviewable's default view, I suspect) that says for libsdformat we're actually compiling all of it by design, even the parts we don't use:

# Generates the library exported to users. The explicitly listed srcs= matches
# upstream's explicitly listed sources. The explicitly listed hdrs= matches
# upstream's private headers. ign.hh and ign.cc are not incorporated in this
# library, but are incorporated into the `ign_sdf_cmdline` library defined
# below.

We've possibly moved away from that pattern for other externals, but it is what we do for libsdformat currently.

@jwnimmer-tri
Copy link
Collaborator


tools/workspace/sdformat/package.BUILD.bazel, line 65 at r1 (raw file):

    linkstatic = 1,
    visibility = ["//visibility:private"],
    deps = ["@tinyxml"],

nit This was actually the only remaining use of @tinyxml in Drake (woohoo!).

You probably should either deprecate that external in this PR, or open an issue for me to do it in a follow-up.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: We don't routinely throw for unsupported SDF features. Or have I missed that?

Ah, good point.

I see above for EMPTY that we can return nullptr, so I'll follow suite with that, then.


tools/workspace/sdformat/package.BUILD.bazel, line 65 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This was actually the only remaining use of @tinyxml in Drake (woohoo!).

You probably should either deprecate that external in this PR, or open an issue for me to do it in a follow-up.

Working: Shall do!

Using 7c96461 as a model, found by sifting through:

git log -S deprecation -- '*.bazel'

tools/workspace/sdformat/package.BUILD.bazel, line 102 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

There's a comment (behind the fold in reviewable's default view, I suspect) that says for libsdformat we're actually compiling all of it by design, even the parts we don't use:

# Generates the library exported to users. The explicitly listed srcs= matches
# upstream's explicitly listed sources. The explicitly listed hdrs= matches
# upstream's private headers. ign.hh and ign.cc are not incorporated in this
# library, but are incorporated into the `ign_sdf_cmdline` library defined
# below.

We've possibly moved away from that pattern for other externals, but it is what we do for libsdformat currently.

OK Thanks for clarifying Jeremy!

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Updated overview to talk about changes more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, good point.

I see above for EMPTY that we can return nullptr, so I'll follow suite with that, then.

Done.


tools/workspace/sdformat/package.BUILD.bazel, line 65 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Shall do!

Using 7c96461 as a model, found by sifting through:

git log -S deprecation -- '*.bazel'

Done. Did not add stub-test (didn't seem worth the time).

Added build_file_deprecation before realizing you had used epilog: 🤷

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

a discussion (no related file):
Working: This PR upgrades to 10.3, not 10.2 🤦


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

a discussion (no related file):
Working: We have some malformed models that now cause failures from 9.x to 10.x.
I should be able to steal them from Addisu's branch.


@EricCousineau-TRI EricCousineau-TRI changed the title Upgrade sdformat to 10.2.0 Upgrade sdformat to 10.3.0 Feb 26, 2021
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: This PR upgrades to 10.3, not 10.2 🤦

Done.


a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: We have some malformed models that now cause failures from 9.x to 10.x.
I should be able to steal them from Addisu's branch.

Done.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

a discussion (no related file):
Working: Anzu PR 6254 needs to land, then I should do a Anzu-master-Drake-PR build to ensure this doesn't break our workflow.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Anzu PR 6254 needs to land, then I should do a Anzu-master-Drake-PR build to ensure this doesn't break our workflow.

Launched Anzu-master-Drake-PR, job 97.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Launched Anzu-master-Drake-PR, job 97.

Done. Passed with one minor failure; fixed in Anzu PR 6427 (which should coincide w/ Drake update).


@SeanCurtis-TRI SeanCurtis-TRI removed the status: single reviewer ok https://drake.mit.edu/reviewable.html label Feb 26, 2021
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.

-(status: single reviewer ok)

I'm knocking off the single reviewer due to the scope growth.

One outstanding issue (not made by this PR, but revealed and exacerbated by it).

Reviewed 6 of 6 files at r2, 3 of 3 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


examples/manipulation_station/models/cupboard.sdf, line 113 at r3 (raw file):

      <child>left_door</child>
      <parent>cupboard_body</parent>
      <pose>-0.008 -0.1395 0 0 0 0</pose>

BTW Huzzah! I hope this is because sdformat no longer fills in new values? :D Please? (Although the fact that the unit tests on the bizarre rgb color specifications aren't failing suggests that's not the case yet. :(


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done.

I noticed a bug.

MakeShapeFromSdfGeometry is called twice, once in detail_scene_graph.cc to construct visual geometries and once in detail_sdf_parser.cc to parse collision geometries (I know, I know).

detail_sdf_parser.cc handles the null case, detail_scene_graph.cc does not. Before it'd only hit this error condition if someone failed to give a geometry that SDF recognized and one that drake recognized. Now it'll happen if someone specifies a visual height map. We probably want to clean that up.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I noticed a bug.

MakeShapeFromSdfGeometry is called twice, once in detail_scene_graph.cc to construct visual geometries and once in detail_sdf_parser.cc to parse collision geometries (I know, I know).

detail_sdf_parser.cc handles the null case, detail_scene_graph.cc does not. Before it'd only hit this error condition if someone failed to give a geometry that SDF recognized and one that drake recognized. Now it'll happen if someone specifies a visual height map. We probably want to clean that up.

I'd like to punt b/c there's nuance here that you know and I missed, and I believe we can just make it throw an exception.

It cool if I just restore the error behavior, and we say "don't care" for now? (we can always relax this strict thing when someone actually cares)

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'd like to punt b/c there's nuance here that you know and I missed, and I believe we can just make it throw an exception.

It cool if I just restore the error behavior, and we say "don't care" for now? (we can always relax this strict thing when someone actually cares)

Er - did you meant to block on this? (can't tell, due to lack of stampzz)

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: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er - did you meant to block on this? (can't tell, due to lack of stampzz)

Yeah. I did intend to block.

If you're going to throw, then you'll also need to change the documentation for the method. It currently doesn't claim it'll throw. It claims nullptr.

On the other hand, I think a very simple change fixes all of this.

Change from (at the bottom of the MakeShapeFromSdfGeometry function in this same file):

  auto instance = make_unique<GeometryInstance>(
      X_LC, MakeShapeFromSdfGeometry(sdf_geometry, resolve_filename),
      sdf_visual.Name());
  instance->set_illustration_properties(
      MakeVisualPropertiesFromSdfVisual(sdf_visual, resolve_filename));
  return instance;

to

  auto shape = MakeShapeFromSdfGeometry(sdf_geometry, resolve_filename);
  if (shape != nullptr) {
    auto instance =
        make_unique<GeometryInstance>(X_LC, move(shape), sdf_visual.Name());
    instance->set_illustration_properties(
        MakeVisualPropertiesFromSdfVisual(sdf_visual, resolve_filename));
    return instance;
  }
  return nullptr;

Modify the documentation to indicate it'll return nullptr for the same reason that MakeShapeFromSdfGeometry does and a TODO in the unit test that an unrecognized visual geometry returns nullptr and we're good.

The only caller of MakeGeometryInstanceFromSdfVisual (in detail_sdf_parser.cc already checks for nullptr.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Yeah. I did intend to block.

If you're going to throw, then you'll also need to change the documentation for the method. It currently doesn't claim it'll throw. It claims nullptr.

On the other hand, I think a very simple change fixes all of this.

Change from (at the bottom of the MakeShapeFromSdfGeometry function in this same file):

  auto instance = make_unique<GeometryInstance>(
      X_LC, MakeShapeFromSdfGeometry(sdf_geometry, resolve_filename),
      sdf_visual.Name());
  instance->set_illustration_properties(
      MakeVisualPropertiesFromSdfVisual(sdf_visual, resolve_filename));
  return instance;

to

  auto shape = MakeShapeFromSdfGeometry(sdf_geometry, resolve_filename);
  if (shape != nullptr) {
    auto instance =
        make_unique<GeometryInstance>(X_LC, move(shape), sdf_visual.Name());
    instance->set_illustration_properties(
        MakeVisualPropertiesFromSdfVisual(sdf_visual, resolve_filename));
    return instance;
  }
  return nullptr;

Modify the documentation to indicate it'll return nullptr for the same reason that MakeShapeFromSdfGeometry does and a TODO in the unit test that an unrecognized visual geometry returns nullptr and we're good.

The only caller of MakeGeometryInstanceFromSdfVisual (in detail_sdf_parser.cc already checks for nullptr.

Working: Thanks! Will do so now.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Thanks! Will do so now.

Done. (w/ minor change)

@EricCousineau-TRI
Copy link
Contributor Author


examples/manipulation_station/models/cupboard.sdf, line 113 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Huzzah! I hope this is because sdformat no longer fills in new values? :D Please? (Although the fact that the unit tests on the bizarre rgb color specifications aren't failing suggests that's not the case yet. :(

OK Ah, good point :(

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.

OOps...missed one thing.

Reviewed 2 of 2 files at r4.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)


examples/manipulation_station/models/cupboard.sdf, line 113 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Ah, good point :(

C'est la vie.


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Modify the documentation to indicate it'll return nullptr for the same reason that MakeShapeFromSdfGeometry does and a TODO in the unit test that an unrecognized visual geometry returns nullptr and we're good.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)


examples/manipulation_station/models/cupboard.sdf, line 113 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

C'est la vie.

Yup. I went ahead and updated the issue from BitBucket to GitHub, and posted on that issue: gazebosim/sdformat#193


multibody/parsing/detail_scene_graph.cc, line 169 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Modify the documentation to indicate it'll return nullptr for the same reason that MakeShapeFromSdfGeometry does and a TODO in the unit test that an unrecognized visual geometry returns nullptr and we're good.

Done - sorry 🤦

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:

I'd recommend another set of eyes for the workspace stuff.

Reviewed 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@sherm1 for platform review, please.

TBH I think the bazel stuff is straightforward. Can always be fixed in follow-up PR.

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

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.

Reviewed 1 of 4 files at r1, 1 of 6 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sherm1)


tools/workspace/pkg_config.bzl, line 307 at r6 (raw file):

        "build_epilog": attr.string(),
        "pkg_config_paths": attr.string_list(),
        "build_file_deprecation": attr.string(),

nit The naming convention here is a bit unlike all of its peers.

We have "build_epilog" to refer to text to place into the BUILD file directly.
But here we have "build_file_deprecation" but it's for just one target, not the whole file.
The target-specific options are named "extra_..." above.

How about calling this "extra_deprecation"?

It's a little awkward in the sense that there is not necessarily any meaning to "non-extra" deprecation (from pkg-config natively), but perhaps its a fair compromise?


tools/workspace/pkg_config.bzl, line 358 at r6 (raw file):

                      that we ignore the environment variable PKG_CONFIG_PATH
                      set by the user.
    deprecation: (Optional) Add a deprecation message to the library BUILD

nit This is named build_file_deprecation not deprecation.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)


tools/workspace/pkg_config.bzl, line 307 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The naming convention here is a bit unlike all of its peers.

We have "build_epilog" to refer to text to place into the BUILD file directly.
But here we have "build_file_deprecation" but it's for just one target, not the whole file.
The target-specific options are named "extra_..." above.

How about calling this "extra_deprecation"?

It's a little awkward in the sense that there is not necessarily any meaning to "non-extra" deprecation (from pkg-config natively), but perhaps its a fair compromise?

Done. I don't have too strong of an opinion, just needed something other than deprecation since that was effectively reserved. Thanks!


tools/workspace/pkg_config.bzl, line 358 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This is named build_file_deprecation not deprecation.

Done. (Oops!)

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.

Reviewed 3 of 3 files at r7.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

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.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sherm1)


tools/workspace/sdformat/repository.bzl, line 23 at r7 (raw file):

            # remove this patch and set SDFORMAT_DISABLE_CONSOLE_LOGFILE=1
            # instead, probably for sdformat10 or so.
            "@drake//tools/workspace/sdformat:no-console-file.patch",

nit Just realized -- this patch files should be git rm in this PR.

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.

Reviewed 3 of 3 files at r7.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sherm1)

Copy link
Member

@sherm1 sherm1 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:. A few nits left.

Reviewed 1 of 4 files at r1, 1 of 6 files at r2, 3 of 3 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 3 of 3 files at r7.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)


tools/workspace/sdformat/package.BUILD.bazel, line 218 at r7 (raw file):

        "src/SDFImplPrivate.hh",
        "src/Scene.cc",
        # "src/ScopedGraph.hh",

nit: is this intentional? If so, why?

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI and @sherm1)


tools/workspace/sdformat/package.BUILD.bazel, line 218 at r7 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: is this intentional? If so, why?

Done. Sorry, that was in preparation for newer libsdformat11 (SDFormat 1.8); this PR just focuses on libsdformat10 (SDFormat 1.7).


tools/workspace/sdformat/repository.bzl, line 23 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Just realized -- this patch files should be git rm in this PR.

Done. Good catch!

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-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, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @SeanCurtis-TRI, and @sherm1)


tools/workspace/tinyxml/repository.bzl, line 19 at r8 (raw file):

        modname = modname,
        pkg_config_paths = pkg_config_paths,
        extra_deprecation = "DRAKE DEPRECATED: The @tinyxml external is being removed from Drake on or after 2021-06-01.  Downstream projects should add it to their own WORKSPACE if needed.",  # noqa

Working: Should move to 2021-07-01.

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.

Reviewed 2 of 3 files at r8.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @SeanCurtis-TRI, and @sherm1)

Deprecate `@tinyxml` external
Ignore new `/sdf//heightmap` elements
models: Fix now-invalid usages of `/sdf//pose`

Co-Authored-By: Addisu Z. Taddese <addisu@openrobotics.org>
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.

Reviewed 3 of 3 files at r8, 1 of 1 files at r9.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r8, 1 of 1 files at r9.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

@EricCousineau-TRI
Copy link
Contributor Author

Current commit is pure fast-forward on latest upstream. Merging.

@EricCousineau-TRI EricCousineau-TRI added the status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits label Mar 2, 2021
@EricCousineau-TRI EricCousineau-TRI merged commit 0c6ac30 into RobotLocomotion:master Mar 2, 2021
@EricCousineau-TRI
Copy link
Contributor Author

Pretty merge:

$ git log HEAD~2..HEAD --graph --oneline --no-decorate
*   0c6ac303fb Merge pull request #14705 from EricCousineau-TRI/feature-libsdformat10
|\  
| * f13c731426 Upgrade sdformat to 10.3.0
| * d045a8cdd4 pkg_config: Add extra_deprecation option
|/  
* f47387b29e [FEM] Add abstract base class for FEM model and state (#14681)
*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants