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

Adds an isRelativePath to determine if path is relative #312

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Feb 16, 2022

🦟 Bug fix

Fixes #

Summary

This PR adds an isRelativePath function to ign-common Filesystem.hh. It is a simple wrapper around std::filesystem. The rationale for this is so we can remove std::filesystem from ign-gazebo as this is causing other issues such as gazebo-tooling/release-tools#639.

Signed-off-by: Arjo Chakravarty [email protected]

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 16, 2022
arjo129 added a commit to gazebosim/gz-sim that referenced this pull request Feb 16, 2022
Depends on gazebosim/gz-common#312

See gazebo-tooling/release-tools#639

Basically, this removes the dependency on `std::filesystem` in  ign-gazebo as its been causing issues when binaries are produced by diffrent compiler versions. This current version makes sure that we keep all `std::filesystem` code in` ign-common`.

Signed-off-by: Arjo Chakravarty <[email protected]>
@nkoenig
Copy link
Contributor

nkoenig commented Feb 16, 2022

You can target this to ign-common4

@mjcarroll
Copy link
Contributor

You can target this to ign-common4

I think it will actually take a separate implementation since the std::filesystem stuff isn't in common4.

I wouldn't be opposed to it, because we'll likely need to fix it there for gazebo5 and gazebo6

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Can I get a few tests just to demonstrate how it should work?

@arjo129
Copy link
Contributor Author

arjo129 commented Feb 16, 2022

You can target this to ign-common4

I think it will actually take a separate implementation since the std::filesystem stuff isn't in common4.

I wouldn't be opposed to it, because we'll likely need to fix it there for gazebo5 and gazebo6

Yeah I think the fix will have to be applied separately for fortress and edifice. Makes sense to get this in first cause its easier. I can tackle edifice and fortress tomorrow morning.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #312 (110befe) into main (4256a57) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 110befe differs from pull request most recent head b9dad99. Consider uploading reports for the commit b9dad99 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage   76.74%   76.74%           
=======================================
  Files          75       75           
  Lines       10323    10325    +2     
=======================================
+ Hits         7922     7924    +2     
  Misses       2401     2401           
Impacted Files Coverage Δ
include/ignition/common/Filesystem.hh 100.00% <ø> (ø)
src/Filesystem.cc 95.45% <100.00%> (+0.06%) ⬆️

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 4256a57...b9dad99. Read the comment docs.

This PR adds an `isRelativePath` function to ign-common Filesystem.hh. It is a simple wrapper around `std::filesystem`.  The rationale for this is so we can remove `std::Filesystem` from ign-gazebo as this is causing other issues such as gazebo-tooling/release-tools#639.

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@mjcarroll mjcarroll enabled auto-merge (squash) February 16, 2022 16:50
@mjcarroll mjcarroll merged commit 7bb95dc into main Feb 16, 2022
@mjcarroll mjcarroll deleted the arjo/feat/add_relative_path branch February 16, 2022 17:07
arjo129 added a commit to gazebosim/gz-sim that referenced this pull request Mar 2, 2022
Depends on gazebosim/gz-common#312

See gazebo-tooling/release-tools#639

Basically, this removes the dependency on `std::filesystem` in  ign-gazebo as its been causing issues when binaries are produced by diffrent compiler versions. This current version makes sure that we keep all `std::filesystem` code in` ign-common`.

Signed-off-by: Arjo Chakravarty <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants