QNX Compatibility #1968

JaiXJM-BB · 2025-01-10T16:10:26Z

Compatibility for the QNX Operating System, versions 7.1 and 8.0. As discussed in #1914 .

Changes in Detail:
Each change will have a GitHub comment attached explaining why it is necessary. Below is an overview of the changes as a whole.
All changes are wrapped in appropriate if(QNX) or if defined(__QNX__) statements.

About half of the CMake changes were to adjust compiling options for all builds to accommodate QNX. These generally already existed for other OSes, and simply needed a check for QNX tacked on as well.
Some install commands were changed to respect CMAKE_INSTALL_INCLUDEDIR if QNX is being compiled for, instead of using include/.
The rest of the CMake changes enable the compilation of tests. This is so that tests can be run on QNX target, as cross-compilation is required and thus using CTest is unideal.
-- There are two environment variables added to support this, both of which only apply if the target is a QNX system. These are $QNX_TARGET_DATASET_DIR and $QNX_BUILD_TESTS. $QNX_TARGET_DATASET_DIR is set to wherever the examples/Data directory will be cloned to on the target when compiling tests to avoid some file location errors. Tests only build if $QNX_BUILD_TESTS is set. (I have these as environment variables as it was slightly cleaner when I was building, but can easily be changed to CMake options if needed)
There are two non-CMake changes here.
-- One adds to a gcc bugfix that is slightly worse on qcc (QNX Compiler)
-- The second switches a placeholder in config.h.in to a different filepaths on QNX.

… for QNX

…reaming)

* GitLab version (branch QNX_7.1_v4.1.1) * ADDED: Build tested with target `install` or `all` if cross compiling for QNX * ADDED: Test Installation when building with QNX (Review this for upstreaming) * UPDATED: Build tests, fixed some unit tests. Floating points still off. * UPDATED: Tests Fix * UPDATED: all non-serialization tests working. * QNX 8.0: Working version. * REMOVED: Removal of test prints * UPDATED: formatting to match, removed commented out testing lines

JaiXJM-BB · 2025-01-10T16:12:24Z

cmake/GtsamTesting.cmake

-				set_target_properties(${script_name} PROPERTIES EXCLUDE_FROM_ALL ON)
+				if(NOT QNX OR NOT DEFINED ENV{QNX_BUILD_TESTS})
+					set_target_properties(${script_name} PROPERTIES EXCLUDE_FROM_ALL ON)
+				endif()


Allows tests to be built when compiling for QNX with option QNX_BUILD_TESTS

Is there something specific to QNX that you need to use an environment variable? We normally do "make check" to run the tests.

JaiXJM-BB · 2025-01-10T16:12:39Z

cmake/GtsamTesting.cmake

-
+			if(NOT QNX OR NOT DEFINED ENV{QNX_BUILD_TESTS})
+				set_target_properties(${target_name} PROPERTIES EXCLUDE_FROM_ALL ON)
+			endif()


Allows tests to be built when compiling for QNX with option QNX_BUILD_TESTS

JaiXJM-BB · 2025-01-10T16:17:14Z

gtsam/3rdparty/ceres/CMakeLists.txt

+    #Install in the install include directory rather than at the install prefix.
+    install(FILES ${ceres_headers} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/gtsam/3rdparty/ceres)
+endif()


There's a number of changes identical to this one, which handle the correct include directory for installation in qnx. Note that CMAKE_INSTALL_INCLUDEDIR should point to include/ by default, so in the future the QNX line could just replace the default one.

What is CMAKE_INSTALL_INCLUDEDIR now by default? If incorrect, should we do a PR on that first?

JaiXJM-BB · 2025-01-10T16:17:59Z

gtsam/CMakeLists.txt

+#For config.in searches
+if(QNX)
+set(QNX_TARGET_DATASET_DIR "$ENV{QNX_TARGET_DATASET_DIR}")
+endif()


Dataset directory is not examples/Data when running tests out of the source tree, so this is required to properly set the path.

JaiXJM-BB · 2025-01-10T16:18:28Z

gtsam/base/CMakeLists.txt

 file(GLOB base_headers_tree "treeTraversal/*.h")
-install(FILES ${base_headers_tree} DESTINATION include/gtsam/base/treeTraversal)
+


slightly rearranged for convenience, but same as the other include directory changes.

JaiXJM-BB · 2025-01-10T16:19:43Z

gtsam/base/std_optional_serialization.h

+// QCC (The QNX GCC-based Compiler) also has this issue, but it also extends to trivial destructor.
+#if defined(__QNX__)
+namespace std { template<> struct is_trivially_destructible<boost::serialization::U> : std::false_type {}; }
+#endif


The https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84075 issue extendes to QNX, but also affects is_trivially_destructible. Luckily it can be fixed in the same way.

JaiXJM-BB · 2025-01-10T16:20:07Z

gtsam/config.h.in

+#else
+//Set toolbox path to the path on the target.
+#define GTSAM_INSTALLED_DATASET_DIR "@QNX_TARGET_DATASET_DIR@/gtsam_examples/Data"
+#endif



Data path changed when testing outside of the source tree on QNX.

JaiXJM-BB · 2025-01-10T16:21:03Z

gtsam/discrete/CMakeLists.txt

@@ -1,7 +1,12 @@
 # Install headers
 set(subdir discrete)
 file(GLOB discrete_headers "*.h")
-install(FILES ${discrete_headers} DESTINATION include/gtsam/discrete)
+# FIXME: exclude headers


This FIXME was not added by me; so I assume it may have been changed in the 430 or so commits I needed to merge into this one to get up to develop. This should be double checked before merging.

JaiXJM-BB · 2025-01-10T16:21:24Z

gtsam/inference/tests/testOrdering.cpp

@@ -303,7 +303,7 @@ TEST(Ordering, MetisLoop) {
  symbolicGraph.push_factor(0, 5);

  // METIS
-#if defined(__APPLE__)
+#if defined(__APPLE__) || defined(__QNX__)


Follows apple ordering in test output.

JaiXJM-BB · 2025-01-10T16:22:02Z

gtsam/nonlinear/tests/testSerializationNonlinear.cpp

  deserializeFromXMLFile(GTSAM_SOURCE_TREE_DATASET_DIR
                         "/../../gtsam/nonlinear/tests/priorFactor.xml",
                         factor_deserialized_xml);
+  #else
+  bool c = deserializeFromXMLFile(
+                        "priorFactor.xml",


Again, a filepath search error on QNX. Changed to look in the same directory now, as outside of source tree.

JaiXJM-BB · 2025-01-10T16:22:38Z

gtsam/symbolic/tests/testSymbolicBayesTree.cpp

@@ -687,7 +687,7 @@ TEST(SymbolicBayesTree, COLAMDvsMETIS) {
  {
    Ordering ordering = Ordering::Create(Ordering::METIS, sfg);
 // Linux and Mac split differently when using Metis
-#if defined(__APPLE__)
+#if defined(__APPLE__) || defined(__QNX__)


Again, ordering results on QNX seem to follow the APPLE results from Metis.

JaiXJM-BB · 2025-01-10T16:23:04Z

gtsam_unstable/discrete/tests/testScheduler.cpp

  string path(TOPSRCDIR "/gtsam_unstable/discrete/examples/");
+  #else
+  string path(""); //Same Directory


another path change when testing out of the source tree.

JaiXJM-BB · 2025-01-10T16:24:37Z

gtsam_unstable/partition/tests/testFindSeparator.cpp

  std::vector<size_t> keys{0, 1, 2, 3, 4};
+  #else
+  //Anything where 2 is before 0 will work.
+  std::vector<size_t> keys{2, 0, 3, 1, 4};


This error was quite interesting; but the summary is that 0 1 2 3 4 will partition into 3-4, 0-1-2 instead of 0-1, 2-3-4 on QNX. This is an equally valid partition, it is just interesting that the tiebreaker seems to be related to ordering here.

dellaert

Two big comments, questions:

Since there are so many changes related to CMAKE_INSTALL_INCLUDEDIR, could we not fix that first in a PR first, if it is fixable?
Can you explain a bit more as to why QNX needs special handling of tests, and how?

dellaert · 2025-01-10T17:39:52Z

cmake/GtsamTesting.cmake

-				set_target_properties(${script_name} PROPERTIES EXCLUDE_FROM_ALL ON)
+				if(NOT QNX OR NOT DEFINED ENV{QNX_BUILD_TESTS})
+					set_target_properties(${script_name} PROPERTIES EXCLUDE_FROM_ALL ON)
+				endif()


Is there something specific to QNX that you need to use an environment variable? We normally do "make check" to run the tests.

dellaert · 2025-01-10T17:42:04Z

gtsam/3rdparty/ceres/CMakeLists.txt

+    #Install in the install include directory rather than at the install prefix.
+    install(FILES ${ceres_headers} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/gtsam/3rdparty/ceres)
+endif()


What is CMAKE_INSTALL_INCLUDEDIR now by default? If incorrect, should we do a PR on that first?

dellaert · 2025-01-10T17:42:31Z

gtsam/3rdparty/ceres/CMakeLists.txt

+    #Install in the install include directory rather than at the install prefix.
+    install(FILES ${ceres_headers} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/gtsam/3rdparty/ceres)
+endif()


Add newline at end

dellaert · 2025-01-10T17:42:48Z

gtsam/3rdparty/metis/include/CMakeLists.txt

+else()
+    #Install in the install include directory rather than at the install prefix.
+    install(FILES metis.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/gtsam/3rdparty/metis)


same comment. And, add newline at end :-)

dellaert · 2025-01-10T17:48:43Z

Re-reading your PR comment: indeed, I'd rather not bring in environment variables as a way to configure, as I don't think we use them in any other case - except maybe if third party libraries look for them.

JaiXJM-BB · 2025-01-10T18:04:27Z

Re-reading your PR comment: indeed, I'd rather not bring in environment variables as a way to configure, as I don't think we use them in any other case - except maybe if third party libraries look for them.

Sounds good; there is nothing that requires an environment variable on QNX's end - these could easily be a CMake Flag or removed entirely.

With regards to make check, this doesn't work for QNX (or at least in my experience I've never seen it done or gotten it to work) since ctest runs the files on whatever machine is compiling, and there isn't a CMake port on QNX or a make utility available either, hence cross-compilation. This is why tests are being compiled into executables, so that users can copy them over and run them manually. It does add a lot of compile time which is why I had them on a toggle.

That being said - We could keep unit test cmake changes on the QNX fork or in a patchfile in the QNX documentation repo - that way environment variables/extra cmake options don't affect your develop branch and any user that does want unit test control can find it elsewhere.

JaiXJM-BB · 2025-01-10T18:06:42Z

For CMAKE_INSTALL_INCLUDEDIR, my understanding is that it defaults to include per https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#module:GNUInstallDirs

Making a separate PR first to fix these is a good idea- there's also CMAKE_INSTALL_LIBDIR, CMAKE_INSTALL_BINDIR, and more that apply as well. Not sure if those are already in use.

dellaert · 2025-01-10T23:30:40Z

Gotcha. Take a look at the windows testing: there we created one executable per “folder”, which cut down on compile time quite a bit.

dellaert · 2025-01-11T18:58:07Z

It seems the default is indeed include, so it seems that the code in your else clause is the correct way, right? In that case, maybe we don't need a new PR, just use remove the if-then-else and we'll merge this new correct way instead. @jlblancoc am I missing something?

JaiXJM-BB added 12 commits November 27, 2024 13:17

GitLab version (branch QNX_7.1_v4.1.1)

ba522a8

ADDED: Build tested with target install or all if cross compiling…

45adc0c

… for QNX

ADDED: Test Installation when building with QNX (Review this for upst…

4eff178

…reaming)

Updated: Build tests, fixed some unit tests. Floating points still off.

b78a296

Tests Fix

e33e05a

QNX Changes

8d17fb3

erroneous change

9d35582

fixing spacing

26b301d

fixing spacing issue

8489ebc

Fixed broken merge

77c5c57

Removed erroneous text

20d72d0

JaiXJM-BB commented Jan 10, 2025

View reviewed changes

dellaert reviewed Jan 10, 2025

View reviewed changes

Merge branch 'borglab:develop' into develop

074f889

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QNX Compatibility #1968

QNX Compatibility #1968

JaiXJM-BB commented Jan 10, 2025

JaiXJM-BB Jan 10, 2025

dellaert Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

dellaert Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025

JaiXJM-BB Jan 10, 2025 •

edited

Loading

dellaert left a comment

dellaert Jan 10, 2025

dellaert Jan 10, 2025

dellaert Jan 10, 2025

dellaert Jan 10, 2025

dellaert commented Jan 10, 2025

JaiXJM-BB commented Jan 10, 2025

JaiXJM-BB commented Jan 10, 2025

dellaert commented Jan 10, 2025

dellaert commented Jan 11, 2025

		file(GLOB base_headers_tree "treeTraversal/*.h")
		install(FILES ${base_headers_tree} DESTINATION include/gtsam/base/treeTraversal)

QNX Compatibility #1968

Are you sure you want to change the base?

QNX Compatibility #1968

Conversation

JaiXJM-BB commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaiXJM-BB Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert commented Jan 10, 2025

JaiXJM-BB commented Jan 10, 2025

JaiXJM-BB commented Jan 10, 2025

dellaert commented Jan 10, 2025

dellaert commented Jan 11, 2025

JaiXJM-BB Jan 10, 2025 •

edited

Loading