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

normalize paths before comparison in rosmsg #1586

Merged
merged 5 commits into from
Feb 6, 2019
Merged

normalize paths before comparison in rosmsg #1586

merged 5 commits into from
Feb 6, 2019

Conversation

kejxu
Copy link
Contributor

@kejxu kejxu commented Jan 23, 2019

this is a minor update to the existing results[0] != path: comparison:
normalize all paths before comparison or adding them into paths to make sure all elements in paths are really unique

  • _get_package_paths will return 2 paths for the same package; for example: C:\\ros-win\\install_isolated\\share\\test_rosmaster and C:/ros-win/install_isolated\\share\\test_rosmaster will both be returned for test_rosmaster, resulting in duplicated output

tools/rosmsg/src/rosmsg/__init__.py Outdated Show resolved Hide resolved
tools/rosmsg/src/rosmsg/__init__.py Outdated Show resolved Hide resolved
if path.replace(os.path.sep, '/') != results[0].replace(os.path.sep, '/'):
paths.append(results[0])
else:
if path_in_workspaces != path:
Copy link
Member

Choose a reason for hiding this comment

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

Undefined variable path_in_workspaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

embarrassed, change to use results[0]

paths.append(results[0])
else:
if path_in_workspaces != path:
paths.append(results[0])
Copy link
Member

Choose a reason for hiding this comment

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

The duplication of control seems unnecessary. This could simple stay a single condition since on platforms where os.path.sep is / the replace() calls are no-ops:

if results and results[0].replace(os.path.sep, '/') != path.replace(os.path.sep, '/'):
    paths.append(results[0])

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 0648d30 into ros:melodic-devel Feb 6, 2019
@kejxu kejxu deleted the rosmsg_normalize_path_before_comparison branch February 6, 2019 18:01
@kejxu
Copy link
Contributor Author

kejxu commented Feb 6, 2019

@dirk-thomas, thanks for merging this change! While not related to this change, here is something I'd love to share and ask for opinions:

the reason for the test failure behind this change is essentially because 1 path is normalized, and the other one is not. this normalization happens during https://github.com/ros/ros_comm/blob/melodic-devel/tools/rosmsg/src/rosmsg/__init__.py#L551

path = rospack.get_path(pkgname)

and inside rospack.get_path, abspath is called

path = os.path.abspath(path)

from python documentation:

os.path.abspath(path)
Return a normalized absolutized version of the pathname path. On most platforms, this is equivalent to calling the function normpath() as follows: normpath(join(os.getcwd(), path)).

I haven't had time to dig in further on the behavior of abspath on Linux, but given our experience working with symlinks, I feel like this might be worth looking into. ROS should probably consider to

  • normalize all paths (if this would also make sense with the presence of symlinks)
  • or, avoid any normalization (which might make sanitizing paths very painful)

@dirk-thomas
Copy link
Member

Due to the size of the code base I can't suggest either of the two approaches. Just blindly applying normalization doesn't sounds appealing to me. Removing it from that specific instance might have unintended side effects. I guess it would need to be tried...

tahsinkose pushed a commit to tahsinkose/ros_comm that referenced this pull request Apr 15, 2019
* normalize paths before comparison in rosmsg

* remove use of normcase and remove path_in_workspaces temp variable

* remove duplicated control

* revert unrelated whitespace changes

* keep order of operands
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