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

Filter out subcomponents in build_components_refresh #8792

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented May 11, 2018

Closes #6464.

Ideally we would continue to work on components still marked as # unpackaged in the list, but for now at least it seems worth locking in this stage before moving on to fix the stragglers.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@jamiesnape for feature review, please.
+@ggould-tri for platform review per schedule, please.

@ggould-tri
Copy link
Contributor

:lgtm:, notes below.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


tools/install/libdrake/build_components.bzl, line 8 at r1 (raw file):

#
# When adding new components to the package, please also add the licenses for
# any new external dependencies to :external_licenses.

BTW -- Author's discretion as it is not part of this PR, but it seems odd to me to put "when adding new components" right after "don't update this list". Perhaps this comment is in the wrong place?


tools/install/libdrake/build_components_refresh.py, line 123 at r1 (raw file):

                one_label, component_labels)
            if sans_colon_label:
                line = '    "{}",'.format(sans_colon_label)

This is going to disturb the sort you do above (line 64) , resulting in surprising sorts like the position of //multibody/rigid_body_plant in the generated file. I'm a little bit surprised that buildifier allowed it.

Perhaps it would be better to sort after this step rather than in _find_libdrake_components?


Comments from Reviewable

@jamiesnape
Copy link
Contributor

:lgtm:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

This comment dates from the very first prototype of the installation and
packaging, where the top-level install list had to explicitly call out the
license files for each external.  However, with the install.bzl rules that we
use now, where license data files are installed by the external itself, and
declared and enforced in the build system, the comment here no longer makes any
sense.  Indeed, the target it refers to no longer exists.
@jwnimmer-tri jwnimmer-tri force-pushed the build-libdrake-filter branch from e0d0727 to 4ea2362 Compare May 11, 2018 14:58
@jwnimmer-tri
Copy link
Collaborator Author

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions.


tools/install/libdrake/build_components.bzl, line 8 at r1 (raw file):

Previously, ggould-tri wrote…

BTW -- Author's discretion as it is not part of this PR, but it seems odd to me to put "when adding new components" right after "don't update this list". Perhaps this comment is in the wrong place?

Done. Wow, that comment was old and very, very stale.


tools/install/libdrake/build_components_refresh.py, line 123 at r1 (raw file):

Previously, ggould-tri wrote…

This is going to disturb the sort you do above (line 64) , resulting in surprising sorts like the position of //multibody/rigid_body_plant in the generated file. I'm a little bit surprised that buildifier allowed it.

Perhaps it would be better to sort after this step rather than in _find_libdrake_components?

Done. Great point. It even fixed a bug (rigid_body_plant should have been commented as unpackaged).


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

I tested this locally; the only difference in the install is that include/drake/multibody/multibody_tree:/parsing/*.h are newly installed, since that package is now mentioned due to the refresh. (The PRs that added that code to master did not run build_components_refresh.)

@jwnimmer-tri jwnimmer-tri merged commit 5c6d933 into RobotLocomotion:master May 11, 2018
@jwnimmer-tri jwnimmer-tri deleted the build-libdrake-filter branch May 11, 2018 16:12
@jamiesnape jamiesnape removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements to libdrake.so list of components
3 participants