Fix symlink install python when shebang line is rewritten #1100

kenji-miyake · 2020-06-29T14:39:42Z

Overview

This PR fixes catkin_install_python when CATKIN_SYMLINK_INSTALL=ON.

~~I'm sending this PR to kinetic-devel because I'm still using melodic, but I believe the same change can be applied to noetic-devel.~~ -> Changed to noetic-devel.

Problem

In catkin_install_python.cmake, new (shebang-replaced) files which are generated by file(WRITE ...), which drops the permissions.

catkin/cmake/catkin_install_python.cmake

Line 42 in 4fe89dd

file(WRITE ${rewritten_file} "${data}")

This disables python-scripts execution with rosrun.

Supplemental

The permissions of symlink files (inside install/) are executable because of install(PROGRAMS ...)

catkin/cmake/catkin_install_python.cmake

Line 53 in 4fe89dd

    
                 install(PROGRAMS "${rewritten_file}" DESTINATION "${ARG_DESTINATION}" ${optional_flag})

However, the real files (inside catkin_generated/installspace) are not executable.

Solution

I just added file(COPY ...) before file(WRITE ...).
aead61d#diff-3cfcccb0a4b1968a8dae7de378653f99R41

I also added a variable installspace in order to avoid overwriting the variable, but I can drop this change from this PR.
8f23bf5

How to test (Melodic)

###################################
##### Launch docker container #####
###################################
$ docker run --rm -it osrf/ros:melodic-desktop-full

##################################################
##### Check result with this patch (working) #####
##################################################
# apt update && apt install -y python-catkin-tools
# mkdir -p ~/catkin_ws/src
# cd ~/catkin_ws/src
# git clone https://github.com/kenji-miyake/catkin.git --depth 1 -b fix-symlink-install-python-kinetic # Actually based on noetic-devel
# git clone https://github.com/ros/ros_tutorials.git --depth 1 -b melodic-devel
# cd ~/catkin_ws
# catkin config --install
# catkin build rospy_tutorials --cmake-args -DCATKIN_SYMLINK_INSTALL=ON

# ls -l ~/catkin_ws/install/share/rospy_tutorials/001_talker_listener
total 24
lrwxrwxrwx 1 root root 76 Jun 29 13:55 README -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/README
lrwxrwxrwx 1 root root 76 Jun 29 13:55 listener -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener
lrwxrwxrwx 1 root root 79 Jun 29 13:55 listener.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
lrwxrwxrwx 1 root root 74 Jun 29 13:55 talker -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker
lrwxrwxrwx 1 root root 77 Jun 29 13:55 talker.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker.py
lrwxrwxrwx 1 root root 92 Jun 29 13:55 talker_listener.launch -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/talker_listener.launch

# ls -l /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
-rwxr-xr-x 1 root root 2407 Jun 29 13:55 /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py

# /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
Unable to register with master node [http://localhost:11311]: master may not be running yet. Will keep trying.

#########################################################
##### Check result without this patch (not working) #####
#########################################################
# cd ~/catkin_ws
# catkin clean -y
# rm -rf ~/catkin_ws/src/catkin
# catkin build rospy_tutorials --cmake-args -DCATKIN_SYMLINK_INSTALL=ON

# ls -l ~/catkin_ws/install/share/rospy_tutorials/001_talker_listener
total 24
lrwxrwxrwx 1 root root 76 Jun 29 14:02 README -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/README
lrwxrwxrwx 1 root root 76 Jun 29 14:02 listener -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener
lrwxrwxrwx 1 root root 79 Jun 29 14:02 listener.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
lrwxrwxrwx 1 root root 74 Jun 29 14:02 talker -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker
lrwxrwxrwx 1 root root 77 Jun 29 14:02 talker.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker.py
lrwxrwxrwx 1 root root 92 Jun 29 14:02 talker_listener.launch -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/talker_listener.launch

# ls -l /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
-rw-r--r-- 1 root root 2407 Jun 29 14:02 /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py

# /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
bash: /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py: Permission denied

How to test (Noetic)

###################################
##### Launch docker container #####
###################################
$ docker run --rm -it osrf/ros:noetic-desktop-full

##################################################
##### Check result with this patch (working) #####
##################################################
# apt update && apt install -y git python3-pip
# pip3 install -U "git+https://github.com/catkin/catkin_tools.git#egg=catkin_tools"
# mkdir -p ~/catkin_ws/src
# cd ~/catkin_ws/src
# git clone https://github.com/kenji-miyake/catkin.git --depth 1 -b fix-symlink-install-python-kinetic # Actually based on noetic-devel
# git clone https://github.com/ros/ros_tutorials.git --depth 1 -b melodic-devel
# cd ~/catkin_ws
# catkin config --install
# catkin build rospy_tutorials --cmake-args -DCATKIN_SYMLINK_INSTALL=ON

# ls -l ~/catkin_ws/install/share/rospy_tutorials/001_talker_listener
total 24
lrwxrwxrwx 1 root root 76 Jun 29 16:13 README -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/README
lrwxrwxrwx 1 root root 76 Jun 29 16:13 listener -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener
lrwxrwxrwx 1 root root 79 Jun 29 16:13 listener.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
lrwxrwxrwx 1 root root 74 Jun 29 16:13 talker -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker
lrwxrwxrwx 1 root root 77 Jun 29 16:13 talker.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker.py
lrwxrwxrwx 1 root root 92 Jun 29 16:13 talker_listener.launch -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/talker_listener.launch

# ls -l /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
-rwxr-xr-x 1 root root 2407 Jun 29 16:13 /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py

# /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
Unable to register with master node [http://localhost:11311]: master may not be running yet. Will keep trying.

#########################################################
##### Check result without this patch (not working) #####
#########################################################
# cd ~/catkin_ws
# catkin clean -y
# rm -rf ~/catkin_ws/src/catkin
# catkin build rospy_tutorials --cmake-args -DCATKIN_SYMLINK_INSTALL=ON

# ls -l ~/catkin_ws/install/share/rospy_tutorials/001_talker_listener
total 24
lrwxrwxrwx 1 root root 76 Jun 29 16:14 README -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/README
lrwxrwxrwx 1 root root 76 Jun 29 16:14 listener -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener
lrwxrwxrwx 1 root root 79 Jun 29 16:14 listener.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
lrwxrwxrwx 1 root root 74 Jun 29 16:14 talker -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker
lrwxrwxrwx 1 root root 77 Jun 29 16:14 talker.py -> /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/talker.py
lrwxrwxrwx 1 root root 92 Jun 29 16:14 talker_listener.launch -> /root/catkin_ws/src/ros_tutorials/rospy_tutorials/001_talker_listener/talker_listener.launch

# ls -l /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
-rw-r--r-- 1 root root 2407 Jun 29 16:14 /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py

# /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py
bash: /root/catkin_ws/build/rospy_tutorials/catkin_generated/installspace/listener.py: Permission denied

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

dirk-thomas · 2020-06-29T15:54:25Z

Please change the target branch to noetic-devel. Only after a patch has been reviewed, merged and released into Noetic it will be considered for backporting.

This seems to be related to the change in #1054. @sloretz Can you please take a look at this.

kenji-miyake · 2020-06-29T16:17:30Z

@dirk-thomas Thank you for your quick response! I've fixed my PR for noetic-devel.

sloretz

LGTM!

If I was going to nitpick, I would say this should set the permissions on the generated file to be executable instead of copying the source file permissions via USE_SOURCE_PERMISSIONS since that's what install(PROGRAMS) would do:

https://cmake.org/cmake/help/v3.5/command/install.html

The PROGRAMS form is identical to the FILES form except that the default permissions for the installed file also include OWNER_EXECUTE, GROUP_EXECUTE, and WORLD_EXECUTE.

dirk-thomas · 2020-07-01T22:47:17Z

I would say this should set the permissions on the generated file to be executable instead of copying the source file permissions via USE_SOURCE_PERMISSIONS

@sloretz Do you have a suggestion how to do that? Afaik there is no way to set permissions on a file in CMake (beyond file(COPY ...) and install(...)).

sloretz · 2020-07-11T00:31:56Z

Afaik there is no way to set permissions on a file in CMake (beyond file(COPY ...) and install(...)).

I think it could be done with file(COPY ... FILE_PERMISSIONS <permissions>...) which I think takes the same permission args as install(...) .

dirk-thomas · 2020-07-13T21:41:38Z

I think it could be done with file(COPY ... FILE_PERMISSIONS <permissions>...) which I think takes the same permission args as install(...) .

@sloretz

The current patch already uses file(COPY ... USE_SOURCE_PERMISSIONS) and afterwards overwrites the content of the file which isn't pretty. I thought that was what your nitpick is about and should be improved (#1100 (review)).

I then asked if you can recommend a function to set the permissions without copying the file (#1100 (comment)).

sloretz · 2020-07-13T21:43:18Z

The current patch already uses file(COPY ... USE_SOURCE_PERMISSIONS) and afterwards overwrites the content of the file which isn't pretty.

I'm proposing using FILE_PERMISSIONS, not USE_SOURCE_PERMISSIONS.

dirk-thomas · 2020-07-13T21:55:21Z

Ok, the nitpick is not about the "hacky copy a file and then overwrite its content anyway" but about enforcing an executable bit - even if the source file doesn't have one.

In the case where the shebang line doesn't have to be rewritten the source file is symlinked and therefore must have the executable bit set already. I would rather not workaround a missing executable bit for the source file only in the case when the shebang line is rewritten.

Otherwise it would magically work when the executable bit is missing for the source file as long as the shebang line is rewritten but as soon as the shebang line is update to not require rewriting anymore the symlinked file wouldn't be executable anymore.

dirk-thomas · 2020-07-13T22:00:43Z

I updated the patch to not use the variable name installspace for a path which is not in the install space: f7cb085. This also keep the diff much smaller.

I also added a comment to describe why the file(COPY ...) is done: 92216e0.

dirk-thomas · 2020-07-13T22:11:26Z

@kenji-miyake Thank you for the patch.

kenji-miyake · 2020-07-14T02:35:45Z

@dirk-thomas Thank you, too.

I updated the patch to not use the variable name installspace for a path which is not in the install space: f7cb085. This also keep the diff much smaller.

Yes, no problem. It's what I mentioned here. I also added a variable installspace in order to avoid overwriting the variable, but I can drop this change from this PR.

I also added a comment to describe why the file(COPY ...) is done: 92216e0.

Thank you, looks nice!

kenji-miyake · 2020-07-14T02:43:37Z

@dirk-thomas By the way, will this be backported to melodic?

dirk-thomas · 2020-07-14T02:54:14Z

By the way, will this be backported to melodic?

Yes, that is what the consider-backport label is used for.

* Refactor catkin_install_python.cmake Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp> * Use source-permissions for rewritten_files Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp> * rename temp variable * add comment Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

dirk-thomas · 2020-07-14T17:30:22Z

Cherry-picked to kinetic-devel in 1cb2018.

Kenji Miyake added 2 commits June 29, 2020 12:57

Refactor catkin_install_python.cmake

2dbd0c7

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Use source-permissions for rewritten_files

30c416d

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

sloretz self-assigned this Jun 29, 2020

kenji-miyake changed the base branch from kinetic-devel to noetic-devel June 29, 2020 16:04

sloretz mentioned this pull request Jul 1, 2020

Fix CATKIN_SYMLINK_INSTALL with add_subdirectory() #1102

Merged

sloretz approved these changes Jul 1, 2020

View reviewed changes

dirk-thomas added bug consider-backport labels Jul 7, 2020

dirk-thomas added 2 commits July 13, 2020 14:59

rename temp variable

f7cb085

add comment

92216e0

dirk-thomas changed the title ~~Fix symlink install python~~ Fix symlink install python when shebang line is rewritten Jul 13, 2020

dirk-thomas merged commit 189598e into ros:noetic-devel Jul 13, 2020

kenji-miyake deleted the fix-symlink-install-python-kinetic branch July 14, 2020 02:35

seanyen mentioned this pull request Jul 22, 2020

[noetic-devel][Windows] Fix file COPY cannot read symlink #1109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symlink install python when shebang line is rewritten #1100

Fix symlink install python when shebang line is rewritten #1100

kenji-miyake commented Jun 29, 2020 •

edited

Loading

dirk-thomas commented Jun 29, 2020

kenji-miyake commented Jun 29, 2020

sloretz left a comment

dirk-thomas commented Jul 1, 2020

sloretz commented Jul 11, 2020

dirk-thomas commented Jul 13, 2020

sloretz commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

kenji-miyake commented Jul 14, 2020

kenji-miyake commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

Fix symlink install python when shebang line is rewritten #1100

Fix symlink install python when shebang line is rewritten #1100

Conversation

kenji-miyake commented Jun 29, 2020 • edited Loading

Overview

Problem

Supplemental

Solution

How to test (Melodic)

How to test (Noetic)

dirk-thomas commented Jun 29, 2020

kenji-miyake commented Jun 29, 2020

sloretz left a comment

Choose a reason for hiding this comment

dirk-thomas commented Jul 1, 2020

sloretz commented Jul 11, 2020

dirk-thomas commented Jul 13, 2020

sloretz commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

dirk-thomas commented Jul 13, 2020

kenji-miyake commented Jul 14, 2020

kenji-miyake commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

dirk-thomas commented Jul 14, 2020

kenji-miyake commented Jun 29, 2020 •

edited

Loading