Attempt at multiarch support (#545) #624

scpeters · 2014-04-28T22:07:58Z

This is an attempt at multiarch support (#545), supercedes
(#604). The dpkg-architecture command is used to identify
an appropriate lib folder suffix to use for LD_LIBRARY_PATH
and PKG_CONFIG_PATH. builder.py and
catkin_generate_environment.cmake are modified
accordingly, though there's still a problem with isolated
devel spaces.

~~It works for catkin_make_isolated --install, but I'm getting the following error when running catkin_make_isolated:~~

@wjwwood

dirk-thomas · 2014-04-28T22:15:12Z

I will add some comments inline.

But the PR seems to fail unit tests anyway and as you mentioned yourself does not yet work in all cases.

dirk-thomas · 2014-04-28T22:16:51Z

cmake/catkin_generate_environment.cmake

+    )
+    if (NOT "${CATKIN_MULTIARCH}" STREQUAL "")
+      set(CATKIN_MULTIARCH_LIB_DESTINATION "['lib', 'lib/${CATKIN_MULTIARCH}']")
+      set(CATKIN_MULTIARCH_PKGCONFIG_DESTINATION "['lib/pkgconfig', 'lib/${CATKIN_MULTIARCH}/pkgconfig']")


These two lines should not repeat the values from lines 14 and 15 but use the variable to avoid duplication. Also there is a variable CATKIN_GLOBAL_LIB_DESTINATION which could be uses instead of using lib.

wjwwood · 2014-04-28T22:18:23Z

But the PR seems to fail unit tests anyway and as you mentioned yourself does not yet work in all cases.

I asked him to open it so I could help him with it.

dirk-thomas · 2014-04-28T22:19:18Z

cmake/templates/_setup_util.py.in

+        if type(subfolders) != type([]):
+            subfolders = [subfolders]
+        for subfolder in subfolders:
+            path = path0


The way path is stored in another variable and then overwritten in the loop looks pretty weird. Please avoid that by just using a different variable inside the loop to build the full path without changing the surrounding loop variable path.

Thanks, I think I fixed it in 232d56b

wjwwood · 2014-04-28T22:24:49Z

If you are using Python to invoke the dpkg-architecture command, then there is no need for the 2> /dev/null. I would invoke it like this in python:

import subprocess
...
out, err = subprocess.Popen(
    ['dpkg-architecture', '-qDEB_HOST_MULTIARCH'],
    stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True).communicate()
# Use the returned data in `out`

scpeters · 2014-04-28T23:20:41Z

The test_setup_util.py test is failing because I've changed the variables that are used to configure _setup_util.py. I've tried modifying this, but I'm having trouble debugging, since it writes to a temporary file that I can't see.

scpeters · 2014-04-29T00:15:53Z

I fixed the test, and I've responded to all the comments except the one indicating that I should check for the existence of dpkg-architecture. I agree; though I'm able to run this code on my mac without any problem, even though it doesn't have dpkg-architecture.

scpeters · 2014-04-29T00:18:53Z

I need to take a little break from this, if someone else is willing to help tackle checking the existence of dpkg-architecture. @j-rivero

j-rivero · 2014-04-29T16:44:33Z

dpkg-architecture is part of dpkg-dev package which seems not be part of a clean ubuntu installation.

Maybe we could use the debian/ubuntu print-multiarch gcc flag (present in all current Ubuntu distributions): gcc -print-multiarch which I believe should be present in all default installations out there.

scpeters · 2014-04-29T17:18:21Z

dpkg-dev may not be part of a clean ubuntu installation, but it is included with build-essential. How much can you do with catkin without build-essential?

j-rivero · 2014-04-29T17:19:15Z

A quick patch against Steve's branch seems to work fine in my system.

_Error cases:_

CMake check for gcc -print-multiarch won't display anything by the output (ERROR_QUIET) and return empty if no gcc is present or no -print-multiarch flag exists in gcc.
Same for python subprocess call, return an empty in cases of error.

scpeters · 2014-04-29T17:31:02Z

Thanks @j-rivero ; I will defer judgment to the catkin maintainers about the patch.

j-rivero · 2014-04-29T17:45:08Z

We can go with the approach of using dpkg-architecture, no a problem. Let me know if that is the option chosen and I will implement the check for the binary.

scpeters · 2014-04-29T21:46:30Z

@wjwwood @dirk-thomas Do you have a preference between dpkg-architecture and gcc -print-multiarch (see Jose's comments above)?

I'll cc @thomas-moulard as well.

wjwwood · 2014-04-29T21:50:35Z

First off, if dpkg-architecture is not available it should never fail. As long as that is the case I'm fine.

If dpkg-architecture is part of build-essential then it will be brought in as part of the base system for building ROS and it shouldn't be a problem. If you believe gcc -print-multiarch is more portable or reliable then I don't mind either one.

dirk-thomas · 2014-04-29T21:55:38Z

Agreeing to Williams comment. Any tool which is either available by default on Debian systems or is being pulled in by the base dependencies of ROS is fine with me. Important is to consider custom configurations: can a user use clang and not have gcc? Or can he remove dpkg-architecture while still having everything else to build ROS packages?

scpeters · 2014-04-30T00:25:00Z

As of trusty, build-essential depends on both dpkg-dev and gcc. An unrealistic edge case: if someone built clang from source without build-essential installed, then neither method would be available.

I personally would lean towards dpkg-architecture.

dirk-thomas · 2014-04-30T00:29:41Z

Sounds good to me.

j-rivero · 2014-04-30T00:43:55Z

I'm without enough caffeine but, what about using a two step approach? First check for gcc -print-multiarch and, if failed, try to use dpkg-architecture so we cover all the possible options.

I have implemented this, the full patch would look like this. The cmake change and the python get_multiarch. And a lovely assert.

scpeters · 2014-04-30T01:05:11Z

@j-rivero I just added your patch. I think it should be enough to cover the case in which those binaries aren't installed, since it should just complain to std_err; I don't think subprocess will throw an exception. The std_out should be empty, and your assertion will verify that it matches the triplet pattern.

j-rivero · 2014-04-30T01:09:11Z

@scpeters in my experiments, when declaring shell=True there is no exception if the binaries are not present.

scpeters · 2014-04-30T01:14:24Z

I just squashed them all into scpeters/catkin@35d9827

j-rivero · 2014-05-05T16:16:32Z

Thanks @scpeters. Let me know if there is anything else missing for merging.

scpeters · 2014-05-05T17:05:37Z

I think it's ready to review.

wjwwood · 2014-05-05T17:08:20Z

I'll try to review it today.

wjwwood · 2014-05-05T21:05:23Z

cmake/catkin_generate_environment.cmake

@@ -10,6 +10,30 @@ function(catkin_generate_environment)
    file(WRITE "${CMAKE_BINARY_DIR}/CATKIN_IGNORE" "")
  endif()

+  # get multiarch name
+  set(CATKIN_MULTIARCH_LIB_DESTINATION "'${CATKIN_GLOBAL_LIB_DESTINATION}'")


From a purely semantic point of view, the name CATKIN_MULTIARCH_LIB_DESTINATION isn't great because it always contains a non arch-specific path (CATKIN_GLOBAL_LIB_DESTINATION) and can have multiple destinations in it. What about replacing CATKIN_MULTIARCH_LIB_DESTINATION with CATKIN_LIB_ENVIRONMENT_PATHS or something like that which is more descriptive of the contents and plural.

Thanks, I changed to CATKIN_*_ENVIRONMENT_PATHS in scpeters/catkin@655012a

dirk-thomas · 2014-05-05T23:14:08Z

python/catkin/builder.py

+    out, err = p.communicate()
+    if p.returncode != 0:
+      out, err = subprocess.Popen(
+          ['dpkg-architecture -qDEB_HOST_MULTIARCH'],


Passing both parts in a single string looks wrong to me. I think it needs to be a list of two string.

I think this is allowed because of shell=True.

You can use a string with whitespace if shell=True, It would be clearer to drop the list. If you have a multi-element list with whitespace it seems to drop the following elements in the list.

I just removed the shell=True and switched to using a list in scpeters/catkin@e99145a. In previous attempts, I had never tried without shell=True, and it only worked as a string with whitespace. It does work, however, with a list and shell=False.

scpeters · 2014-05-05T23:51:57Z

Ok, I think that's all the comments and travis is happy. Thanks for the reviews and let me know if anything else needs work.

dirk-thomas · 2014-05-06T19:17:37Z

cmake/catkin_generate_environment.cmake

+    endif()
+    if (NOT "${CATKIN_MULTIARCH}" STREQUAL "")
+      set(CATKIN_LIB_ENVIRONMENT_PATHS "['${CATKIN_GLOBAL_LIB_DESTINATION}', os.path.join('${CATKIN_GLOBAL_LIB_DESTINATION}', '${CATKIN_MULTIARCH}')]")
+      set(CATKIN_PKGCONFIG_ENVIRONMENT_PATHS "[os.path.join('${CATKIN_GLOBAL_LIB_DESTINATION}', 'pkgconfig'), os.path.join('${CATKIN_GLOBAL_LIB_DESTINATION}', '${CATKIN_MULTIARCH}', 'pkgconfig')]")


Please update these two lines to not repeat the same value again but extend the value like this:

set(foo "[${foo}, newstuff]")

I fixed it and then resquashed.

@j-rivero

This implements multiarch support (ros#545), and supercedes (ros#604). A two step approach uses gcc -print-multiarch and, if failed, fallback to use dpkg-architecture to identify an appropriate lib folder suffix to use for LD_LIBRARY_PATH and PKG_CONFIG_PATH. builder.py and catkin_generate_environment.cmake are modified accordingly. Thanks to @j-rivero for his contributions, which are hidden in this squashed commit.

Attempt at multiarch support (#545)

dirk-thomas · 2014-05-06T20:35:40Z

Thank you, Steve (and everybody else), for your work on this!

thomas-moulard · 2014-05-09T05:01:04Z

Thank you! 👍

dirk-thomas · 2014-05-25T19:13:30Z

This PR broke catkin_make_isolated:

KeyError: 'CATKIN_LIB_ENVIRONMENT_PATHS'
KeyError: 'CATKIN_PKGCONFIG_ENVIRONMENT_PATHS'

I fixed it in f89f4e8.

scpeters · 2014-05-25T22:03:27Z

Ok, thanks for fixing it. Maybe a test should be added for invoking from sym-linked folders?

dirk-thomas · 2014-05-25T23:14:11Z

Sorry, I linked the wrong ticket number - the fix was f89f4e8. The command catkin_make_isolated did not work anymore.

j-rivero · 2014-05-26T09:03:57Z

Sounds to me like we should define a regression test to avoid situations like this one, where we were not able to detect the problem before commit.

dirk-thomas · 2014-05-26T16:05:37Z

I completely agree with having a unit test. But you should still run the tools affected by a change manually and not solely rely on the tests to cover it. In this case any invocation of cmi would have exposed the regression.

dirk-thomas · 2014-05-27T18:34:18Z

Both cases covered now: abf3fcb and 284bee3

j-rivero · 2014-05-27T18:56:02Z

Nice! thanks @dirk-thomas

Try to detect arch-specific library folders using gcc and dpkg-architecture, then add to LD_LIBRARY_PATH and PKG_CONFIG_PATH. Closes catkin#154. Based on ros/catkin#624

scpeters mentioned this pull request Apr 28, 2014

Handle arch-specific library folders #545 (Not ready for merging) #604

Closed

dirk-thomas reviewed Apr 28, 2014
View reviewed changes

wjwwood reviewed May 5, 2014
View reviewed changes

dirk-thomas reviewed May 5, 2014
View reviewed changes

dirk-thomas added the enhancement label May 5, 2014

dirk-thomas reviewed May 6, 2014
View reviewed changes

dirk-thomas added a commit that referenced this pull request May 6, 2014

Merge pull request #624 from scpeters/multiarch

e1da2a2

Attempt at multiarch support (#545)

dirk-thomas merged commit e1da2a2 into ros:indigo-devel May 6, 2014

dirk-thomas added a commit that referenced this pull request May 25, 2014

fix catkin_make_isolated (regresion of #624)

f89f4e8

scpeters deleted the multiarch branch May 25, 2014 22:02

scpeters mentioned this pull request Mar 9, 2015

Catkin tools don't handle arch-specific library folders catkin/catkin_tools#154

Closed

scpeters mentioned this pull request Mar 10, 2015

Add arch-specific library folders (#154) catkin/catkin_tools#156

Merged

j-rivero mentioned this pull request Dec 15, 2015

Add option to enable GNU Install Dirs ros/console_bridge#19

Merged

wjwwood mentioned this pull request Apr 27, 2017

Remaining ros patches from 4.1.1 rtv/Stage#78

Merged

meyerj mentioned this pull request Mar 21, 2018

Always build OpenCV with CMAKE_BUILD_TYPE=Release (fix #14) ros-gbp/opencv3-release#15

Merged

Attempt at multiarch support (#545) #624

Attempt at multiarch support (#545) #624

Conversation

scpeters commented Apr 28, 2014

dirk-thomas commented Apr 28, 2014

Choose a reason for hiding this comment

wjwwood commented Apr 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Apr 28, 2014

scpeters commented Apr 28, 2014

scpeters commented Apr 29, 2014

scpeters commented Apr 29, 2014

j-rivero commented Apr 29, 2014

scpeters commented Apr 29, 2014

j-rivero commented Apr 29, 2014

scpeters commented Apr 29, 2014

j-rivero commented Apr 29, 2014

scpeters commented Apr 29, 2014

wjwwood commented Apr 29, 2014

dirk-thomas commented Apr 29, 2014

scpeters commented Apr 30, 2014

dirk-thomas commented Apr 30, 2014

j-rivero commented Apr 30, 2014

scpeters commented Apr 30, 2014

j-rivero commented Apr 30, 2014

scpeters commented Apr 30, 2014

j-rivero commented May 5, 2014

scpeters commented May 5, 2014

wjwwood commented May 5, 2014

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

scpeters commented May 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented May 6, 2014

thomas-moulard commented May 9, 2014

dirk-thomas commented May 25, 2014

scpeters commented May 25, 2014

dirk-thomas commented May 25, 2014

j-rivero commented May 26, 2014

dirk-thomas commented May 26, 2014

dirk-thomas commented May 27, 2014

j-rivero commented May 27, 2014