Skip to content
New issue

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

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

Already on GitHub? # to your account

Avoid duplication of / in joinPaths (Windows) #201

Merged
merged 10 commits into from
Apr 16, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Apr 14, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🦟 Bug fix

Fixes #

Related with this issue gazebosim/gz-fuel-tools#178

Summary

When we use JoinPaths with a empty string we can have some extra ' /' which generates some issues on Windows. As a explained here. This PR remove the '/' at the beginning and end of the string.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added bug Something isn't working Windows Windows support labels Apr 14, 2021
@ahcorde ahcorde requested a review from mjcarroll April 14, 2021 07:24
@ahcorde ahcorde self-assigned this Apr 14, 2021
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Apr 14, 2021
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #201 (3798dea) into ign-common3 (72820ee) will decrease coverage by 0.00%.
The diff coverage is 84.61%.

❗ Current head 3798dea differs from pull request most recent head 4b27357. Consider uploading reports for the commit 4b27357 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3     #201      +/-   ##
===============================================
- Coverage        75.08%   75.08%   -0.01%     
===============================================
  Files               72       72              
  Lines            10261    10273      +12     
===============================================
+ Hits              7704     7713       +9     
- Misses            2557     2560       +3     
Impacted Files Coverage Δ
include/ignition/common/Filesystem.hh 100.00% <ø> (ø)
src/Filesystem.cc 73.98% <84.61%> (+0.69%) ⬆️
graphics/src/SkeletonAnimation.cc 42.46% <0.00%> (-1.37%) ⬇️

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 72820ee...4b27357. Read the comment docs.

@chapulina
Copy link
Contributor

Can we add some test cases that demonstrate the fix? Also, should we make the behavior consistent across platforms?

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 14, 2021

Also, should we make the behavior consistent across platforms?

I can remove the / at the end of the string, but at the beginning of the string in an Unix like system is a bad idea because I can modify an absolute path

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 15, 2021

MacOS warnings are unrelated

@ahcorde ahcorde requested a review from chapulina April 15, 2021 15:26
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor

I pushed a test in 6c4f5d6, LGTM if it passes.

ahcorde added 5 commits April 15, 2021 22:28
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 16, 2021

MacOS warnings are unrelated

@ahcorde ahcorde enabled auto-merge (squash) April 16, 2021 14:26
@ahcorde ahcorde disabled auto-merge April 16, 2021 14:27
@ahcorde ahcorde enabled auto-merge (squash) April 16, 2021 14:27
@mjcarroll mjcarroll disabled auto-merge April 16, 2021 14:30
@mjcarroll mjcarroll merged commit cdfbe87 into ign-common3 Apr 16, 2021
@mjcarroll mjcarroll deleted the ahcorde/improve/windows/joinPaths branch April 16, 2021 14:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
📜 blueprint Ignition Blueprint bug Something isn't working 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants