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

Support absolute Windows paths with forward slashes in common::FindFile #389

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Jul 9, 2022

🦟 Bug fix

Summary

While working on gazebosim/gz-sim#1574, some tests were failing with error:

    Start 43: UNIT_ign_TEST

43: Test command: C:\src\ign-gazebo\build\bin\UNIT_ign_TEST.exe "--gtest_output=xml:C:/src/ign-gazebo/build/test_results/UNIT_ign_TEST.xml"
43: Environment variables:
43:  IGN_CONFIG_PATH=C:/src/ign-gazebo/build/test/conf
43:  IGN_GAZEBO_SYSTEM_PLUGIN_PATH=C:/src/ign-gazebo/build/bin
43: Test timeout computed to be: 240
43: Running main() from C:\src\ign-gazebo\test\gtest\src\gtest_main.cc
43: [==========] Running 3 tests from 1 test suite.
43: [----------] Global test environment set-up.
43: [----------] 3 tests from CmdLine
43: [ RUN      ] CmdLine.Server
43: Running command [ C:/Users/STraversaro/AppData/Local/mambaforge/envs/igngaz/Library/bin/ign gazebo -s  -r -v 4 --iterations 5 C:/src/ign-gazebo/test/worlds/plugins.sdf]
43: C:\src\ign-gazebo\src\ign_TEST.cc(73): error: Expected: (output.find("iteration " + std::to_string(i))) != (std::string::npos), actual: 18446744073709551615 vs 18446744073709551615
43: [Msg] Ignition Gazebo Server v6.10.0
43: [Err] [D:\bld\libignition-common4_1656525324646\work\src\SystemPaths.cc:379] Unable to find file with URI [C:///src/ign-gazebo/test/worlds/plugins.sdf]
43: [Err] [D:\bld\libignition-common4_1656525324646\work\src\SystemPaths.cc:469] Could not resolve file [C:/src/ign-gazebo/test/worlds/plugins.sdf]
43: [Err] [C:\src\ign-gazebo\src\Server.cc:109] Failed to find world [C:/src/ign-gazebo/test/worlds/plugins.sdf]

Apparently what was happening is that the C:/src/ign-gazebo/test/worlds/plugins.sdf path was interpreted as an URI, resulting in the strange error on C:///src/ign-gazebo/test/worlds/plugins.sdf . While technical C:/ could be an URI with the one-letter scheme C, in practice I am not aware of any one letter URI scheme, and on the other hand many Windows programs supports both absolute paths with backslash and forward slashes, see for example Python:

(igngaz) C:\src\ign-gazebo\build>type hello.py
print("Hello world")

(igngaz) C:\src\ign-gazebo\build>python C:\src\ign-gazebo\build\hello.py
Hello world

(igngaz) C:\src\ign-gazebo\build>python C:/src/ign-gazebo/build/hello.py
Hello world

To support this use case, this PR modifies the common::FindFile function to first check if a file that starts with C:/ or similar is an existing absolute file in the system. If such a file does not exist, then the search procedure falls back to consider it an URI.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@traversaro traversaro requested a review from mjcarroll as a code owner July 9, 2022 12:26
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 9, 2022
@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #389 (d7141c4) into ign-common4 (edb3509) will increase coverage by 0.00%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##           ign-common4     #389   +/-   ##
============================================
  Coverage        77.03%   77.03%           
============================================
  Files               76       76           
  Lines            10735    10736    +1     
============================================
+ Hits              8270     8271    +1     
  Misses            2465     2465           
Impacted Files Coverage Δ
src/SystemPaths.cc 86.83% <90.90%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edb3509...d7141c4. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants