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

Do not ignore metapackages #596

Merged
merged 1 commit into from
Feb 25, 2020
Merged

Do not ignore metapackages #596

merged 1 commit into from
Feb 25, 2020

Conversation

meyerj
Copy link
Contributor

@meyerj meyerj commented Feb 3, 2020

Simple fix for #418. Another related issue is #543.

Metapackages must not be ignored by catkin_tools. The fact that other non-metapackages cannot depend on them according to REP-140 does not imply that they do not need to be built and installed at all.

The question on which packages are selected if a list of package names is given at the command line (or as a whitelist) is a different story in my opinion. This is more what issue #366 is about, but not directly connected to this pull request. I will open a separate PR for that.

Metapackages must not be ignored by catkin_tools. The fact that other non-metapackages
cannot depend on them according to REP-140 does not imply that they do not need to be
built and installed at all.
@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Feb 3, 2020

Can you describe a use-case for building and installing them?

The contents of metapackages is strictly regulated, their purpose is fixed to allowing group-installs of other packages, other non-metapackages are not allowed to depend on them and when installed, only a package.xml is placed in share/$pkg_name.

Is it the latter that you are interested in?


Edit: just read #418 (comment), which seems to give a partial rationale for why you want them installed.

@meyerj
Copy link
Contributor Author

meyerj commented Feb 3, 2020

Can you give a use-case for building and installing them?

The contents of metapackages is strictly regulated, their purpose is fixed to allowing group-installs of other packages, other non-metapackages are not allowed to depend on them and when installed, only a package.xml is placed in share/$pkg_name.

Is it the latter that you are interested in?

Yes, that is the primary intention. Because only metapackages are allowed to depend on metapackages it has no consequences if catkin build ignores them in most cases, but there is also no good reason to do so and it is also not an implication of REP-140. As far as I know, catkin_make and catkin_make_isolated do not treat metapackages in any special way, both for finding recursive dependencies nor for actually building them. The only difference is that they create a default CMakeLists.txt for metapackages from a template if the package does not provide one (ros/catkin#349).

One valid scenario, where the current behavior of ignoring metapackages is broken, is with two workspaces, where the second one has a metapackage that depends on a metapackage in the first one:

workspace_a
\-- src
    |-- package_a1
    |-- package_a2
    \-- metapackage_a    (exec_depend on package_a1 and package_a2)

workspace_b    (extends workspace_a/devel or workspace_a/install)
\-- src
   |-- package_b1
   \-- metapackage_b   (exec_depend on package_b1 and metapackage_a)

There are some existing examples of metapackages depending on other metapackages, e.g. desktop/package.xml.

@meyerj
Copy link
Contributor Author

meyerj commented Feb 3, 2020

The question on which packages are selected if a list of package names is given at the command line (or as a whitelist) is a different story in my opinion. This is more what issue #366 is about, but not directly connected to this pull request. I will open a separate PR for that.

I found that catkin_tools does already consider all types of dependencies when selecting packages to be built, which is what I would expect. Only the name of functions get_cached_recursive_build_depends_in_workspace() and get_recursive_build_depends_in_workspace() is confusing, because they actually return all recursive build_depends, buildtool_depends, test_depends and run_depends since 6bc286a.

@mikepurvis
Copy link
Member

Looks great, thanks for the contribution.

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.

3 participants