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

TPE #7

Merged
merged 5 commits into from
Jul 30, 2020
Merged

TPE #7

merged 5 commits into from
Jul 30, 2020

Conversation

chapulina
Copy link
Contributor

Closes #5

These are the files I see on my colcon workspace:

Installed TPE files
install/include/ignition/physics2/ignition/physics/tpe-plugin
install/include/ignition/physics2/ignition/physics/tpe-plugin/Export.hh
install/include/ignition/physics2/ignition/physics/tpe-plugin/detail
install/include/ignition/physics2/ignition/physics/tpe-plugin/detail/Export.hh
install/include/ignition/physics2/ignition/physics/tpelib
install/include/ignition/physics2/ignition/physics/tpelib/Export.hh
install/include/ignition/physics2/ignition/physics/tpelib/detail
install/include/ignition/physics2/ignition/physics/tpelib/detail/Export.hh
install/lib/libignition-physics2-tpelib.so.2
install/lib/pkgconfig/ignition-physics2-tpelib.pc
install/lib/pkgconfig/ignition-physics2-tpe.pc
install/lib/pkgconfig/ignition-physics2-tpe-plugin.pc
install/lib/ign-physics-2/engine-plugins/libignition-physics-tpe-plugin.so
install/lib/ign-physics-2/engine-plugins/libignition-physics2-tpe-plugin.so.2.1.0
install/lib/ign-physics-2/engine-plugins/libignition-physics2-tpe-plugin.so.2
install/lib/ign-physics-2/engine-plugins/libignition-physics2-tpe-plugin.so
install/lib/libignition-physics2-tpe-plugin.so.2.1.0
install/lib/libignition-physics2-tpelib.so.2.1.0
install/lib/libignition-physics2-tpe-plugin.so.2
install/lib/cmake/ignition-physics2-tpe
install/lib/cmake/ignition-physics2-tpe/ignition-physics2-tpe-targets.cmake
install/lib/cmake/ignition-physics2-tpe/ignition-physics2-tpe-config.cmake
install/lib/cmake/ignition-physics2-tpe/ignition-physics2-tpe-config-version.cmake
install/lib/cmake/ignition-physics2-tpelib
install/lib/cmake/ignition-physics2-tpelib/ignition-physics2-tpelib-targets.cmake
install/lib/cmake/ignition-physics2-tpelib/ignition-physics2-tpelib-config-version.cmake
install/lib/cmake/ignition-physics2-tpelib/ignition-physics2-tpelib-config.cmake
install/lib/cmake/ignition-physics2-tpelib/ignition-physics2-tpelib-targets-relwithdebinfo.cmake
install/lib/cmake/ignition-physics2-tpe-plugin
install/lib/cmake/ignition-physics2-tpe-plugin/ignition-physics2-tpe-plugin-config-version.cmake
install/lib/cmake/ignition-physics2-tpe-plugin/ignition-physics2-tpe-plugin-targets-relwithdebinfo.cmake
install/lib/cmake/ignition-physics2-tpe-plugin/ignition-physics2-tpe-plugin-config.cmake
install/lib/cmake/ignition-physics2-tpe-plugin/ignition-physics2-tpe-plugin-targets.cmake
install/lib/libignition-physics2-tpe-plugin.so
install/lib/libignition-physics2-tpelib.so

There are 2 main libraries:

  • tpe-plugin
  • tpelib

Some files seem not to belong to either though:

  • install/lib/cmake/ignition-physics2-tpe/*
  • install/lib/pkgconfig/ignition-physics2-tpe.pc

So I haven't included them anywhere. I think dartsim is including the equivalent files on its side though. Maybe these should be added to tpe-plugin?

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@j-rivero
Copy link
Collaborator

Some files seem not to belong to either though:

* `install/lib/cmake/ignition-physics2-tpe/*`

* `install/lib/pkgconfig/ignition-physics2-tpe.pc`

Not sure if I understand what are these files designed for. As a first sight they should be included in the corresponding -dev package.

@chapulina
Copy link
Contributor Author

Not sure if I understand what are these files designed for.

I'm not sure either. I think they're just artefacts of how we're using ign-cmake to create components. Dartsim has the same issue, with both ignition-physics1-dartsim-plugin.pc and ignition-physics1-dartsim.pc, where dartsim-plugin is the string used for includes and the .so, but the component itself is called dartsim. Do we know which of these pc / cmake files people are actually using?

@chapulina chapulina changed the title Tpe TPE May 29, 2020
@scpeters
Copy link
Contributor

each component creates .cmake and .pc files

the dartsim folder has two components defined:

on is for the headers it installs, and the other is for the plugin

@chapulina
Copy link
Contributor Author

chapulina commented May 29, 2020

Ah that's what I was missing, @scpeters . I see includes for both components though, maybe the dartsim-plugin autogenerated ones are not needed?

install/include/ignition/physics2/ignition/physics/dartsim/World.hh
install/include/ignition/physics2/ignition/physics/dartsim-plugin/Export.hh
install/include/ignition/physics2/ignition/physics/dartsim-plugin/detail/Export.hh

I guess since TPE-plugin is not installing any headers, we could get rid of the tpe component?

https:/ignitionrobotics/ign-physics/blob/c7f99dec07881a62008f16732253039e35053f88/tpe/plugin/CMakeLists.txt#L3-L5

I'm not sure what that may break through, because the tpe component is the only one mentioned here (as opposed to tpe-plugin and tpelib):

https:/ignitionrobotics/ign-physics/blob/c7f99dec07881a62008f16732253039e35053f88/CMakeLists.txt#L87-L88

And the components in ign_configure_build seem to need to match the directories under src. Maybe some updates to ign-cmake will be needed to cover these cases? And for now we could proceed with the dummy(?) tpe component?

@scpeters
Copy link
Contributor

I see includes for both components though, maybe the dartsim-plugin autogenerated ones are not needed?

The dartsim-plugin Export.hh files just define visibility symbols, which shouldn't be needed for a plugin. I think our debbuilder job will be marked as unstable if there are uninstalled files. Maybe we want to be able to tell ign-cmake to disable generation of these headers? They are currently only disabled for INTERFACE libraries / components.

install/include/ignition/physics2/ignition/physics/dartsim-plugin/Export.hh
install/include/ignition/physics2/ignition/physics/dartsim-plugin/detail/Export.hh

I guess since TPE-plugin is not installing any headers, we could get rid of the tpe component?

I need to double-check what the tpe component is doing. I think we have that component because the name of the root folder is tpe

If you rename the root folder to tpelib, you should be able to side-step the tpe component

@iche033
Copy link

iche033 commented May 29, 2020

Maybe we want to be able to tell ign-cmake to disable generation of these headers?

+1, yeah I agree we don't need to install these auto-generated headers for tpe plugin as well.

Looks like the tpe component may just be an extra wrapper around the tpelib component. I copied off the dartsim CMakeLists.txt when the work first started so maybe we can remove it if it's not necessary.

@j-rivero
Copy link
Collaborator

To avoid warnings on unwanted files to be installed, we can use the debian/not-installed file

@chapulina
Copy link
Contributor Author

Ok, for now I just followed what DART is doing and included it in tpe-plugin: 69c1cc9

@j-rivero
Copy link
Collaborator

Testing this branch in a custom job not to upload anything: https://build.osrfoundation.org/job/_test_ign-physics2-debbuilder/

@j-rivero
Copy link
Collaborator

Testing this branch in a custom job not to upload anything: https://build.osrfoundation.org/job/_test_ign-physics2-debbuilder/

I tried to build 2.1.0 with this branch and the software is not installing engine-* directories. I have seen this problem before but never explore the root cause of it:

dh_install: warning: Cannot find (any matches for) "usr/lib/*/ign-physics-[0-99]/engines/libignition-physics*-tpe-plugin.so" (tried in ., debian/tmp)

dh_install: warning: libignition-physics2-tpe-dev missing files: usr/lib/*/ign-physics-[0-99]/engines/libignition-physics*-tpe-plugin.so
dh_install: warning: Cannot find (any matches for) "usr/lib/*/ign-physics-[0-99]/engine-plugins/libignition-physics*-tpe-plugin*.so.*" (tried in ., debian/tmp)

dh_install: warning: libignition-physics2-tpe missing files: usr/lib/*/ign-physics-[0-99]/engine-plugins/libignition-physics*-tpe-plugin*.so.*
dh_install: error: missing files, aborting
make[1]: *** [debian/rules:15: override_dh_install] Error 2

@j-rivero
Copy link
Collaborator

I tried to build 2.1.0 with this branch and the software is not installing engine-* directories. I have seen this problem before but never explore the root cause of it:

agh said nothing. I was trying to build code without tpe PRs merged. Let's see how it goes:
Build Status

@j-rivero
Copy link
Collaborator

Solved the problems with engine paths https://build.osrfoundation.org/job/_test_ign-physics2-debbuilder/5/

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.

tpe files are not installed, making debbuilds unstable
4 participants