Fix working with filesystem parent paths.#112
Merged
clalancette merged 3 commits intomasterfrom Nov 16, 2020
Merged
Conversation
Contributor
Author
|
CI:
|
mjcarroll
reviewed
Nov 5, 2020
I found two separate bugs while working with parent paths: 1. Asking for the first parent path of an absolute path would add a duplicate separator at the beginning. I fixed this by checking to see if the separator already exists, and skiping if there is already one. 2. Asking for the grandparent path of an absolute path would return a relative path, which is very incorrect. The problem here turned out to be that we were not propagating empty vector pieces from a child to a parent, and so the grandparent would miss out that it was supposed to be absolute. Fix this by always adding all pieces to the parents, even when they are empty. I also added tests for the above cases. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
clalancette/fix-filesystem-parent-paths
branch
from
9d70878 to
49aed47
Compare
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Contributor
Author
|
All right, I do believe I've fixed the Windows issues. Kicking off another CI to make sure:
|
Contributor
Author
|
@mjcarroll @mjeronimo I could use another review here, thanks. |
Contributor
Author
|
All of the CI failures on Windows are known to be flaky, and have failed in the last several Windows builds. So with green CI everywhere else, and an approval, I'll go ahead and merge. Thanks for the reviews. |
Contributor
|
It looks like this PR may have introduced an issue on windows debug. It's not immediately obvious to me where this error is occurring though. |
Contributor
Author
|
Sigh, yes. Thanks for pointing it out, I'll take a look. |
clalancette
added a commit
that referenced
this pull request
Nov 17, 2020
This was a bug introduced by #112, but only shows up on Windows Debug for some reason. Regardless, we should not try to index into an empty string. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Contributor
Author
|
Windows Debug was right, and I had a bad bug in this patch. Fix in #113 |
clalancette
added a commit
that referenced
this pull request
Nov 18, 2020
This was a bug introduced by #112, but only shows up on Windows Debug for some reason. Regardless, we should not try to index into an empty string. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I found two separate bugs while working with parent paths:
would add a duplicate separator at the beginning. I fixed
this by checking to see if the separator already exists,
and skiping if there is already one.
would return a relative path, which is very incorrect. The
problem here turned out to be that we were not propagating
empty vector pieces from a child to a parent, and so the
grandparent would miss out that it was supposed to be absolute.
Fix this by always adding all pieces to the parents, even
when they are empty.
I also added tests for the above cases.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org
This is currently blocking the resolution of ros/pluginlib#212 . I'll run CI on everything above rcpputils just to ensure we aren't breaking something.