Skip to content
This repository has been archived by the owner on Feb 4, 2020. It is now read-only.

build prefix command is naive #57

Closed
wjwwood opened this issue Oct 26, 2015 · 7 comments
Closed

build prefix command is naive #57

wjwwood opened this issue Oct 26, 2015 · 7 comments
Assignees

Comments

@wjwwood
Copy link
Contributor

wjwwood commented Oct 26, 2015

This function:

https://github.com/ament/ament_tools/blob/master/ament_tools/build_types/cmake.py#L346-L364

Currently sources dependencies of the package being built in an arbitrary order if they have a local_setup.* file.

Instead it should source the local_setup.* files for the recursive build dependencies and they should be sourced in topological order.

@dirk-thomas dirk-thomas added the ready Work is about to start (Kanban column) label Oct 27, 2015
@dirk-thomas dirk-thomas self-assigned this Oct 27, 2015
@dirk-thomas
Copy link
Contributor

build_dependencies in this context (

for path in context.build_dependencies:
) are not the direct build dependencies of the package specified in the manifest. This list already contains all recursive dependencies in topological order (determined by
def topological_order_packages(
).

Therefore I don't think anything needs to be changed here?

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed ready Work is about to start (Kanban column) labels Oct 28, 2015
@dirk-thomas
Copy link
Contributor

Based on the output of #62 it looks like that the order is correct. The problem is that the environment files include not only the recursive dependencies but also downstream packages - basically all packages of the workspace.

@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 28, 2015
@wjwwood
Copy link
Contributor Author

wjwwood commented Oct 28, 2015

@dirk-thomas But they should be skipped because they don't have environment hooks inside the workspace? or is it a problem when you install two workspaces to the same install space?

@dirk-thomas
Copy link
Contributor

Sorry, I meant downstream packages. E.g. ament_cppcheck doesn't have any dependencies but currently uses the environment of all packages in the workspace (which is only the case when rebuilding a workspace again).

@wjwwood
Copy link
Contributor Author

wjwwood commented Oct 28, 2015

Oh, it's not just the dependencies, but also anything that comes before in the topological order.

@dirk-thomas
Copy link
Contributor

It includes even packages which come afterwards in the topological order.

@dirk-thomas
Copy link
Contributor

For packages with dependencies on other ament packages the list and order is correct. But for packages without dependencies on other ament packages the list contains all packages from the workspace. The fix is simple: #63.

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 28, 2015
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Oct 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants