Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows][kinetic] Fix devel and install space intermixes in %PATH% after devel\setup.bat is sourced #1010

Merged
merged 2 commits into from
May 31, 2019

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented May 31, 2019

This change is to address an issue originally reported here: ms-iot/ROSOnWindows#69

On Windows, we observed that %PATH% intermixes devel and install space for bin and lib. Ideally the devel\bin and devel\lib should always come before the install space after devel\setup.bat is sourced. Otherwise, for example, at runtime, it could favor the content from install space over the one in devel space and subsequently cause unexpected problems, especially when we build\test a package both existed in devel space and install space.

This change is to propose to make sure bin and lib on Windows for ENV_VAR_SUBFOLDERS['PATH'] in _setup_util.py, and then let _setup_util.py to prepend the workspace prefix accordingly. In this case, _setup_util.py can produce %PATH% like:
C:\catkin_ws\devel\bin;C:\catkin_ws\devel\lib;C:\opt\ros\melodic\x64\bin;C:\opt\ros\melodic\x64\lib;...

@dirk-thomas
Copy link
Member

I am a bit confused how this patch relates to the described "devel / install mixup" described. Can you please elaborate what you are referring to with that.

@seanyen
Copy link
Contributor Author

seanyen commented May 31, 2019

@dirk-thomas

Sorry for any confusions if I messed up on catkin terminology.

What I am talking about is that, before this change, say you have a ROS installation under c:\opt\ros\melodic\x64\, then you create a workspace, checkout some code, and do a catkin_make. Then you want to do some testing, so devel\setup.bat is sourced. Now you check %PATH%, and it will be something like C:\catkin_ws\devel\lib;C:\opt\ros\melodic\x64\lib;C:\opt\rosdeps\x64\lib;C:\catkin_ws\devel\bin;C:\opt\ros\melodic\x64\bin;C:\opt\rosdeps\x64\bin;.... And as you can notice, the lib paths with overlapping prefixes go first before all bin paths.

In this case, say if there is a package I am working on, which already existed on C:\opt\ros\melodic\x64 prefix and placed a DLL under lib, and then I try to correct it to place it to bin and test my change in devel workspace. At runtime, due to the DLL search order honors the path order in %PATH%, the DLL under C:\opt\ros\melodic\x64\lib will be found first, instead of the new location I placed for testing, which is C:\catkin_ws\devel\bin. That's one example where we hit problems due to the search order.

That's why I figured the correct order should be more like C:\catkin_ws\devel\bin;C:\catkin_ws\devel\lib;C:\opt\ros\melodic\x64\bin;C:\opt\ros\melodic\x64\lib;..., where bin and lib should always go together in order of overlapping prefix.

@dirk-thomas
Copy link
Member

Thank you for the contribution as well as the clarification. Now I understand how the patch works.

@dirk-thomas dirk-thomas merged commit 9702882 into ros:kinetic-devel May 31, 2019
@seanyen seanyen deleted the windows_fix_bin_lib branch June 1, 2019 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants