Skip to content
This repository has been archived by the owner on Dec 14, 2022. It is now read-only.

Dev build does not include CppUnitLite #15

Closed
varunagrawal opened this issue Aug 5, 2021 · 12 comments
Closed

Dev build does not include CppUnitLite #15

varunagrawal opened this issue Aug 5, 2021 · 12 comments

Comments

@varunagrawal
Copy link

If I install libgtsam-dev, I should also be able to do #include <CppUnitLite/TestHarness.h> as we include a copy of it in GTSAM.

Unfortunately this is not the case, so a downstream application fails when we try to include CppUnitLite.

fatal error: CppUnitLite/TestHarness.h: No such file or directory
 #include <CppUnitLite/TestHarness.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

Can we double check if CppUnitLite is being added and if not, what updates to CMake would be required to include it.

@varunagrawal
Copy link
Author

varunagrawal commented Aug 5, 2021

I looked at my local install and make install does indeed install CppUnitLite:

Install the project...
-- Install configuration: "Release"
-- Installing: /usr/local/lib/cmake/GTSAM/gtsam_extra.cmake
-- Installing: /usr/local/lib/cmake/GTSAM/GTSAMConfig.cmake
-- Installing: /usr/local/lib/cmake/GTSAM/GTSAMConfigVersion.cmake
-- Installing: /usr/local/lib/cmake/GTSAM/GTSAM-exports.cmake
-- Installing: /usr/local/lib/cmake/GTSAM/GTSAM-exports-release.cmake
-- Up-to-date: /usr/local/include/CppUnitLite/Failure.h
-- Up-to-date: /usr/local/include/CppUnitLite/Test.h
-- Up-to-date: /usr/local/include/CppUnitLite/TestHarness.h
-- Up-to-date: /usr/local/include/CppUnitLite/TestRegistry.h
-- Up-to-date: /usr/local/include/CppUnitLite/TestResult.h
-- Installing: /usr/local/lib/libCppUnitLite.a
-- Up-to-date: /usr/local/include/gtsam/global_includes.h
-- Up-to-date: /usr/local/include/gtsam/precompiled_header.h

@borglab-launchpad
Copy link

OK, so CppUnitLite is installed, but the header files are not found?
From what I understand about cmake: if the downstream application requires CppUnitLite, it should explicitly do a "find_package()" on CppUnitLite, and list a dependency on ("target_link_library" or similar) such that the path to the CppUnitLite files becomes visible. It should not rely on GTSAM's dependencies on CppUnitLite.
Having that said I don't understand how the application (if it does indeed list gtsam as dependency) would not inherit the path to CppUnitLite via it's dependencies on GTSAM. This is maybe a question for @jlblancoc

@jlblancoc
Copy link
Member

jlblancoc commented Aug 15, 2021

From what I understand about cmake: if the downstream application requires CppUnitLite, it should explicitly do a "find_package()"

Correct.

Actually, it's doubtful that gtsam needs to / should install CppUnitLite in first place, since it's only required for unit tests, not for running user apps linking to gtsam, right, @varunagrawal ??

Bottom line on the error anyway is that the library (.a) and headers are installed, but its cmake targets (xxx-config.cmake, xxx-targets.cmake) are not, which are the files a user program will need for find_package(CppUnitLite) to work; but, again, I think it's not the responsibility of GTSAM to install CppUnitLite ;-)
Not even for our private PPA, but absolutely not if we plan to submit gtsam to Debian / Ubuntu official repos! :-)

@jlblancoc
Copy link
Member

Addressing original OP's problem: the best way to solve the problem in your downstream program is to either add CppUnitLite as a git submodule or using ExternalProject.

@varunagrawal
Copy link
Author

From what I understand about cmake: if the downstream application requires CppUnitLite, it should explicitly do a "find_package()"

Correct.

Actually, it's doubtful that gtsam needs to / should install CppUnitLite in first place, since it's only required for unit tests, not for running user apps linking to gtsam, right, @varunagrawal ??

Bottom line on the error anyway is that the library (.a) and headers are installed, but its cmake targets (xxx-config.cmake, xxx-targets.cmake) are not, which are the files a user program will need for find_package(CppUnitLite) to work; but, again, I think it's not the responsibility of GTSAM to install CppUnitLite ;-)
Not even for our private PPA, but absolutely not if we plan to submit gtsam to Debian / Ubuntu official repos! :-)

Oh okay. CppUnitLite doesn't have a git repo and the original web page is broken. If a CMake command fixes this, I'll gladly close it.

@varunagrawal
Copy link
Author

I think there was some confusion here (which is my fault). So CppUnitLite comes bundled with GTSAM under the 3rdparty directory. When I install GTSAM locally via CMake, installation of CppUnitLite is automatically taken care of and is thus available on my system.

However, when I install GTSAM via apt-get, on a fresh system CppUnitLite is not installed at all, thus the 2 installation candidates are not the same, and this leads to errors in the #include as aforementioned. Ideally, the dev build of GTSAM should install CppUnitLite as well since that's what cmake .. && make install does. Does this make sense?

As for a use case, GTDynamics uses GTSAM's CppUnitLite to run write and execute unit tests. As a developer, if I want to follow the same paradigm, I should be able to, which is what a dev version implies. I don't think using CMake to find_package(CppUnitLite) in a downstream application would help here since there is nothing to find as it is not installed at all.

@varunagrawal
Copy link
Author

I guess what I'm trying to say is, we have some new students working on GTDynamics this Fall semester, so they should not have to locally compile GTSAM since they are not touching GTSAM code. Doing a sudo apt-get install libgtsam-dev should give them the same result as if they cloned and compiled the repo (and apt-get install should be preferred since precompiled binaries are awesome and you folks are awesome for maintaining them 🙂).

@berndpfrommer
Copy link
Collaborator

As an outsider (only really user) of gtsam I have to ask: why does the cmake install target install CppUnitLite if it's not required to compile/link against GTSAM? Are unit tests supposed to be installed? I thought they just run when you "make test", but never make it outside of the source tree.
And related to this: do your new students need to run the unit tests?

@varunagrawal
Copy link
Author

Unit tests are not installed but the tools used to write them are. This makes sense since you can easily build testable extensions for code that depends on GTSAM.

@berndpfrommer
Copy link
Collaborator

I'd love to say "yes" at this point but packaging CppUnitLite along with GTSAM(or anything else that is not absolutely needed for GTSAM), however convenient right now, is not the right direction to go from a packaging perspective. At the moment the biggest show stopper we have from turning GTSAM into a official ubuntu package is libmetis where apparently gtsam is using some internal interfaces so we can't use the standard libmetis package. We have an ugly collision of header files, and I had to rename libmetis to something like libmetis_gtsam to avoid errors during packaging. Packaging CppUnitLite along with gtsam will create the same headaches in the future should somebody else decide they want to create a proper, separate CppUnitLite package.

Would having a separate CppUnitLite debian package being an acceptable option? I can look into making one but I would not want to have the gtsam-dev package depend on it.

Also, in your emails above, what are the "tools used to write them" that are installed along with GTSAM? Can they also be removed as gtsam install targets? When it comes to packaging, less is more :)

@varunagrawal
Copy link
Author

I see what you mean. That takes this conversation in a different direction which I am very happy with.

One thing we can do is host our own repo for CppUnitLite that I can clone in the CI. This should be pretty fast and easy (due to the lack of dependencies) and take off the burden of generating a whole package. This will also allow for the future possibility of using git submodules in GTSAM.

As for that metis issue, yeah I can relate. I haven't heard back from the metis devs which is incredibly frustrating.

When I mentioned "tools used to write them" I basically meant CppUnitLite but that term can also include all the 3rdparty modules.

@varunagrawal
Copy link
Author

Okay update on this: I have managed to get a working version of CppUnitLite in a separate repo which can be easily installed via CMake. However, there seems to be some other subtle issues, for which I will open another issue since it is not related to this.

Thank you for all the responses @berndpfrommer and @jlblancoc

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

4 participants