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

Replace rcutils_get_file_size with rcpputils::fs::file_size #291

Merged

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Feb 8, 2020

Changes

  • Replaced calls to rcutils_file_size with rcpputils::fs::file_size
  • Replace calls to std::remove with rcpputils::fs::remove

Notes:

  • rcutils_file_size returns 0ul when the file does not exist while rcpputils::fs::file_size throws. Because of this, all changes in this PR must check exists before invoking file_size.
  • In tests, I opted to using ASSERT_TRUE on rcpputils::fs::path::exists instead of branching on it before invoking rcpputils::fs::path::file_size. All asserts include a message to state that the expected path does not exist.

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 marked this pull request as ready for review February 8, 2020 00:16
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Copy link
Collaborator

@Karsten1987 Karsten1987 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 green CI

@@ -71,7 +71,8 @@ std::vector<uint8_t> get_input_buffer(const std::string & uri)
throw std::runtime_error{errmsg.str()};
}

const auto decompressed_buffer_length = rcutils_get_file_size(uri.c_str());
const auto file_path = rcpputils::fs::path{uri};
const auto decompressed_buffer_length = file_path.exists() ? file_path.file_size() : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might want to use 0u here and below. You've done this in the change before as well.

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/84b3f6c9c3a5a7c447eff6ed7c5b18fd/raw/d5a65e3e3ed40040ffaa519608c28fb29e86de84/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_compression rosbag2_storage_default_plugins rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_compression rosbag2_storage_default_plugins rosbag2_tests
Job: ci_launcher

@emersonknapp
Copy link
Collaborator

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

@zmichaels11
Copy link
Contributor Author

It looks like the macOS CI is currently broken due to infrastructure.
I think we should wait on merging this until that can build.

@zmichaels11
Copy link
Contributor Author

Restarting CI:

  • Linux-aarch64 Build Status

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 12, 2020

Status update:
macOS build farm is still failing. I'd still like to hold off on merging this for now since it could break stuff.

Edit:
Restarting CI:

  • macOS Build Status

@zmichaels11
Copy link
Contributor Author

Merging because this is passing on all systems.

@zmichaels11 zmichaels11 merged commit fafbe76 into ros2:master Feb 13, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/use-rcpputils-fs-file_size branch February 13, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants