Skip to content

Separate compilation units#44

Merged
dirk-thomas merged 2 commits intomasterfrom
separate_compilation_units
Dec 6, 2016
Merged

Separate compilation units#44
dirk-thomas merged 2 commits intomasterfrom
separate_compilation_units

Conversation

@dirk-thomas
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas commented Dec 6, 2016

Fixes #40. Connect to #40.

I propose to merge this PR first. I am happy to look into updating the existing PRs since this one moves quite some code around.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Dec 6, 2016
@dirk-thomas dirk-thomas self-assigned this Dec 6, 2016
@dhood
Copy link
Copy Markdown
Member

dhood commented Dec 6, 2016

what would be the recommended build invocation now? the same as before but without the -j1?

@dirk-thomas
Copy link
Copy Markdown
Member Author

Yes, same as before. To use j1 or not still depends on the available resources. The build will now use much less resources per compilation unit. But if you run it with potentially eight cores it might still need more then the system has.

@dhood
Copy link
Copy Markdown
Member

dhood commented Dec 6, 2016

ok so without modification to the packaging job build invocations, do I understand correctly that this should allow the failing linux packaging job to run again without having the RAM increased? (I have launched http://ci.ros2.org/view/packaging/job/packaging_linux/501/ to check)

@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented Dec 6, 2016 via email

@dirk-thomas
Copy link
Copy Markdown
Member Author

Yes, the packaging job passes with this patch.

Copy link
Copy Markdown
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

lgtm!


std::shared_ptr<FactoryInterface>
get_factory(const std::string & ros1_type_name, const std::string & ros2_type_name)
get_factory_@(ros2_package_name)(const std::string & ros1_type_name, const std::string & ros2_type_name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I get the feeling that this might also one day suffer from the non-1:1 mapping situation mentioned here (I am not 100% sure). either way I don't think it should block it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The non-1:1 problem doesn't apply here. This patch only groups all specialization by their ROS 2 package name. Every mapping always has exactly one ROS 2 package associated with it. If there are multiple mapping for the same type within the package that is being handled within the generated conditions which compare the passed string values.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thank you for the explanation!

@dirk-thomas dirk-thomas merged commit 00968ca into master Dec 6, 2016
@dirk-thomas dirk-thomas deleted the separate_compilation_units branch December 6, 2016 23:27
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Dec 6, 2016
This was referenced Dec 6, 2016
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