Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Conversation

@lorenzopivetta
Copy link

Add cmake option TANGO_JPEG_MMX to enable/disable MMX support (default true). Self-defined symbol PPC still required to disable building cpp_test_suite (does not compile on Ubuntu 7.10 PPC)

Comment on lines 214 to 219
if (TANGO_JPEG_MMX STREQUAL "false")
message("MMX support disabled")
else (TANGO_JPEG_MMX STREQUAL "false")
message("Building MMX support; use '-DTANGO_JPEG_MMX=false' to disable")
set(TANGO_JPEG_MMX true)
endif (TANGO_JPEG_MMX STREQUAL "false")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be simpler to use CMake's option (and the usual ON/OFF values) here.
Please check the documentation: https://cmake.org/cmake/help/latest/command/option.html

See some examples:

option(WARNINGS_AS_ERRORS "Treat compiler warnings as errors" OFF)
option(USE_PCH "Use precompiled header for server/tango.h" OFF)

option(TANGO_USE_USING_NAMESPACE "Use \"using namespace\" in header files (deprecated, but ON for backwards compatibility.)." ON)

Copy link
Author

Choose a reason for hiding this comment

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

Much better, indeed! I'll fix it Monday.

Copy link
Author

@lorenzopivetta lorenzopivetta Jan 20, 2020

Choose a reason for hiding this comment

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

@mliszcz, fixed in acc962b. Tested on PPC and x86_64 (Ubuntu 18.04)

@bourtemb
Copy link
Member

Travis CI failed on Debian 8 with the following error:

CMake Error at configure/cmake_linux.cmake:3 (add_library):

  Unrecognized generator expression:

    $<$<BOOL:true>:$<TARGET_OBJECTS:jpeg_mmx_objects>>

Call Stack (most recent call first):

  CMakeLists.txt:43 (include)

CMake Error at configure/cmake_linux.cmake:20 (add_library):

  Unrecognized generator expression:

So it looks like this PR is requiring a minimal CMake version which is more recent than the one provided on Debian 8 (3.0.2 I think).
So we need to agree on this minimal CMake version and update the following line in our main CMakeList.txt if we agree we should raise the minimum required CMake version:

https://github.com/tango-controls/cppTango/blob/tango-9-lts/CMakeLists.txt#L1

The build is passing on Debian 7 because CMake 3.10 is installed in our Docker Debian 7 because the default one was too old.

@t-b
Copy link
Collaborator

t-b commented Jan 17, 2020

@bourtemb That should not be a problem as #653 will require a newer cmake version. We just need to merge #653 first.

Edit: I just saw that this is targetting the backports branch.

@t-b
Copy link
Collaborator

t-b commented Jan 17, 2020

@bourtemb According to https://cmake.org/cmake/help/v2.8.12/cmake.html it has bool support.

@bourtemb
Copy link
Member

I think it can be ok to require a more recent CMake version even for the 9.3-backports branch because it should still be possible to install more recent versions of CMake on old systems as we did when we installed CMake 3.10 on our Travis Debian 7 Docker.

@lorenzopivetta
Copy link
Author

We had cmake 3.6.0 compiled on those old Ubuntu 7.10 PPC

@bourtemb
Copy link
Member

@bourtemb According to https://cmake.org/cmake/help/v2.8.12/cmake.html it has bool support.

I think the issue we are facing here is the lack of support for nested CMake generator expressions in old CMake versions.

@t-b
Copy link
Collaborator

t-b commented Jan 20, 2020

Okay for not having to deal with nested operator expressions here is what I would do:

In that way we would still generate and link to the jpg_mmx object files but these would be just empty.

@lorenzopivetta Here is the code I'm imagining (completely untested):

diff --git a/cppapi/server/jpeg_mmx/CMakeLists.txt b/cppapi/server/jpeg_mmx/CMakeLists.txt
index 927e902c..a034661a 100644
--- a/cppapi/server/jpeg_mmx/CMakeLists.txt
+++ b/cppapi/server/jpeg_mmx/CMakeLists.txt
@@ -20,9 +20,12 @@ if(WIN32)
         target_compile_definitions(jpeg_mmx_objects_sta PRIVATE _64BITS)
     else()
         target_compile_options(jpeg_mmx_objects_dyn PRIVATE -O0)
-        target_compile_definitions(jpeg_mmx_objects_dyn PRIVATE JPG_USE_ASM)
         target_compile_options(jpeg_mmx_objects_sta PRIVATE -O0)
-        target_compile_definitions(jpeg_mmx_objects_sta PRIVATE JPG_USE_ASM)
+
+        if(TANGO_JPEG_MMX)
+          target_compile_definitions(jpeg_mmx_objects_dyn PRIVATE JPG_USE_ASM)
+          target_compile_definitions(jpeg_mmx_objects_sta PRIVATE JPG_USE_ASM)
+        endif()
     endif()

 else(WIN32)
@@ -34,7 +37,10 @@ else(WIN32)
         target_compile_definitions(jpeg_mmx_objects PRIVATE _64BITS)
     else()
         target_compile_options(jpeg_mmx_objects PRIVATE -O0)
-        target_compile_definitions(jpeg_mmx_objects PRIVATE JPG_USE_ASM)
+
+        if(TANGO_JPEG_MMX)
+          target_compile_definitions(jpeg_mmx_objects PRIVATE JPG_USE_ASM)
+        endif()
     endif()

 endif(WIN32)

@t-b
Copy link
Collaborator

t-b commented Jan 23, 2020

@lorenzopivetta
Copy link
Author

We already have automake support for disabling jpegmmx, see https://github.com/tango-controls/TangoSourceDistribution/blob/9ba331bab50ff7d9f2ca47207080a0c71ff3c7ca/assets/configure.ac#L423

Yes. That used to work. But isn't automake been dropped in favor of cmake? Am I missing something?

@t-b
Copy link
Collaborator

t-b commented Jan 23, 2020

@lorenzopivetta For cppTango yes we only use cmake. But for the tango source distribution we still use automake (and don't cmake), but 9.3.4 will be the last tango source distribution release using automake.

@lorenzopivetta
Copy link
Author

Okay for not having to deal with nested operator expressions here is what I would do:

@t-b thanks for looking at this. But, is the version with nested operators definitely dropped?
I'll try this one and let you know.

@t-b
Copy link
Collaborator

t-b commented Jan 23, 2020

nested operators works only on tango-9-lts branch as that allows cmake 3.7, 9.3-backports only allows cmake 2.8.12.

@t-b
Copy link
Collaborator

t-b commented Mar 4, 2020

@lorenzopivetta I think this PR is obsolete as we included that in #674 and #668. Please reopen if I'm wrong.

@t-b t-b closed this Mar 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants