Skip to content

Make sure that an existing path is a directory for create_directories#98

Merged
ivanpauno merged 3 commits intoros2:masterfrom
christophebedard:fs-create-directories-check-if-dir-2
Oct 5, 2020
Merged

Make sure that an existing path is a directory for create_directories#98
ivanpauno merged 3 commits intoros2:masterfrom
christophebedard:fs-create-directories-check-if-dir-2

Conversation

@christophebedard
Copy link
Copy Markdown
Member

@christophebedard christophebedard commented Sep 26, 2020

2nd try for #95 after it was reverted in #96.

I opened a PR to update the failing rosbag2 tests ros2/rosbag2#526, as suggested here #97 (review)

@christophebedard
Make sure that an existing path is a directory for create_directories
1ba186f 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard requested a review from a team as a code owner September 26, 2020 13:12
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Sep 26, 2020

cc @wjwwood @clalancette @fujitatomoya

fujitatomoya
fujitatomoya approved these changes Sep 28, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with a minor comment.

@christophebedard
Add test call to fs::create_directories with empty path
a903aaa 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard mentioned this pull request Sep 28, 2020
Merged
fujitatomoya
fujitatomoya approved these changes Sep 28, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@christophebedard
Check status variable first to possibly avoid calling is_directory()
2e03153 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
christophebedard
christophebedard commented Sep 28, 2020
View reviewed changes
@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Sep 29, 2020

CI (build up to rcpputils, test rcpputils):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Sep 29, 2020

@ivanpauno I think it would be good to use --packages-above-and-dependencies rcpputils for build & test here given what happened last time (#96). Also rosbag2 tests will fail without ros2/rosbag2#526 😅

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Sep 29, 2020

@ivanpauno I think it would be good to use --packages-above-and-dependencies rcpputils for build & test here given what happened last time (#96). Also rosbag2 tests will fail without ros2/rosbag2#526 😅

This ^

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Sep 29, 2020

@ivanpauno I think it would be good to use --packages-above-and-dependencies rcpputils for build & test here given what happened last time (#96). Also rosbag2 tests will fail without ros2/rosbag2#526

Ah ok, thanks for letting me know.
CI can be rerun when the rosbag2 PR is approved and ready.

@christophebedard christophebedard mentioned this pull request Sep 30, 2020
Merged
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Oct 1, 2020

Running CI since the rosbag2 PR is approved (and ready I assume).

#98 + ros2/rosbag2#526, --packages-above-and-dependencies rcpputils:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@christophebedard christophebedard mentioned this pull request Oct 1, 2020
Merged
@christophebedard
Copy link
Copy Markdown
Member Author

christophebedard commented Oct 2, 2020 •
edited
Loading

I see those rclpy failures in another similar job (12519) + they're (seemingly unrelated) warnings, so I think CI looks good.

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Oct 5, 2020

New windows CI (unrelated failures were fixed):

  • Windows Build Status

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Oct 5, 2020

mmm, it seems that I checked the wrong nightlies, because it's still failing.
Anyways, those failures are completely unrelated, so merging this one.

@ivanpauno ivanpauno merged commit 1973577 into ros2:master Oct 5, 2020
@christophebedard christophebedard deleted the fs-create-directories-check-if-dir-2 branch October 5, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@christophebedard @ivanpauno @wjwwood @fujitatomoya