Update rcutils_calculate_directory_size() to support recursion #306

Barry-Xu-2018 · 2020-10-29T07:34:01Z

No description provided.

Barry-Xu-2018 · 2020-10-29T07:42:05Z

Related to #304

fujitatomoya · 2020-10-30T02:36:10Z

@clalancette @Karsten1987 friendly ping. i will be looking into this today.

include/rcutils/filesystem.h

fujitatomoya · 2020-10-30T03:08:06Z

test/test_filesystem.cpp

@@ -419,9 +419,9 @@ TEST_F(TestFilesystemFixture, calculate_directory_size) {
 #ifdef WIN32


I would keep the non-recursive test case like following.

non-recursive test case: rcutils_calculate_directory_size(test/dummy_folder/dummy-subfolder/dummy-subfolder)

recursive test case: rcutils_calculate_directory_size(test/dummy_folder)

This modification is to support recursion. So not support non-recursion.
If there is non-recursive test case, do you mean 2 API --- one for non-recursion and the other for recursion ?

my bad, i wasn't clear enough. what i mean is not about the function but test cases,

test only files in a single directory. (non-recursive search test case)

test directories and files. (recursive search test case)

fujitatomoya

lgtm

include/rcutils/filesystem.h

clalancette

I still have the overall concern about this, which is whether this change makes a difference to rosbag2. I'm still waiting to hear back from @Karsten1987 about that. Other than that, I have one more concern inline.

include/rcutils/filesystem.h

src/filesystem.c

Barry-Xu-2018 · 2020-11-12T06:24:35Z

@clalancette @fujitatomoya

I updated codes to implement recursion without consuming memory from stack.
A new API rcutils_calculate_directory_size_with_recursion() is added. For the original API rcutils_calculate_directory_size(), the behavior isn't changed.

clalancette

I've got a bunch of stuff to fix in here.

One other thing; the inner loops for both the Windows and Unix sides are very similar. Do you think it is possible to use a common function for that?

include/rcutils/filesystem.h

src/filesystem.c

include/rcutils/filesystem.h

src/filesystem.c

Barry-Xu-2018 · 2020-11-16T05:45:06Z

if we cannot change the rosbag2 code, i think we should stick with current rcutils_calculate_directory_size that means it returns 0 even with error. but as interface, i think current interface is the one which needs to be changed. since user application cannot tell error or actual size is 0. that would lead the another problem.

Agree. So I try to change the interface to return -1 for error.
Now only one project (rosbag2) use this rcutils_calculate_directory_size().
So the sooner we update this interface, the better.

Barry-Xu-2018 · 2020-11-16T07:40:29Z

@clalancette @fujitatomoya

I updated codes based on your comments.

Change the return type from a size_t to an ssize_t. Then we can return -1 to indicate error.

I adopt this way to show error while calling rcutils_calculate_directory_size() or rcutils_calculate_directory_size_with_recursion().

clalancette

A few more things to think about in here. This one is tricky to get right.

src/filesystem.c

include/rcutils/filesystem.h

Barry-Xu-2018 · 2020-11-19T11:07:09Z

@clalancette

Sorry to late update.
Please help review again.
For rcutils_calculate_directory_size(), I don't keep the same interface as rcutils_calculate_directory_size_with_recursion().
It is because we should update code of rosbag once rcutils_calculate_directory_size() interface is changed.
I cannot make sure 2 repositories have been update at the same time for CI system. (Otherwise, CI system always reports build failure).

clalancette

One major thing to change. We are getting closer, thanks for all of the revisions.

include/rcutils/filesystem.h

Barry-Xu-2018 · 2020-11-20T03:20:41Z

@clalancette

Updated. Please review again.

include/rcutils/filesystem.h

clalancette · 2020-11-20T14:38:41Z

src/filesystem.c

 size_t
 rcutils_calculate_directory_size(const char * directory_path, rcutils_allocator_t allocator)


I understand that it is hard to make API changes across repositories, but I do think we should do that here. In point of fact, this API has the same problems as the new rcutils_calculate_directory_size_with_recursion had, namely:

You can't tell if the return value is 0 because the call failed, or because there were just no files in the directory.

On 32-bit, you can only return a size up to 4GB, which is pretty small.

So I think we should change this signature to:

rcutils_ret_t rcutils_calculate_directory_size(const char *directory_path, uint64_t *size, rcutils_allocator_t allocator)

And also update rosbag2 to use that signature as well. It won't pass the PR job until we release both of them, but that's OK. We'll run normal CI (that's why we have it), and then I can merge both and do a release of both at the same time.

Okay. I will update rcutils_calculate_directory_size().
And update rosbag2' codes since rcutils_calculate_directory_size() is changed.

We'll run normal CI (that's why we have it)

I don't know this normal CI. What is the difference with the current system ？

I don't know this normal CI. What is the difference with the current system ？

Sorry, I should have been more clear.

We actually have two separate types of CI. When you open a PR (and anytime you update it), the "Rpr" job runs on https://build.ros2.org. You can see the output of that down below in the "Checks" section of this PR. The PR job takes all of the current Rolling packages that this package depends on, installs them, checks this branch out, and then does a build-and-test. This is great as it is automatic, and gives us fairly quick feedback on small issues like compiler warnings, linter warnings, unit tests, etc. But it does have some problems:

It won't test packages that depend on this package. So in this case, the PR job succeeded, but if we merge this and then try to build rosbag2, it would fail because of the changed API.

If there are API changes in packages that this package depends on, it will just fail. There is no way to do a coordinated test across several repositories.

It only tests Linux, not the other Tier-1 platforms.

For all of those reasons, we also have https://ci.ros2.org, where we can test all of the things above.

Therefore, for this PR, once we are happy with the code and it is approved, I can run a full test on ci.ros2.org to get cross-platform tests going, and with all packages that depend on this one.

Does that all make some sense?

Thank you so much for your detailed explanation.
I get it.
I try to login https://ci.ros2.org and I think normal user have no permission to execute it.
So you will help to execute it. Right ?

So you will help to execute it. Right ?

Yes, exactly. Once I approve, I'll go ahead and run the job on https://ci.ros2.org.

include/rcutils/filesystem.h

Barry-Xu-2018 · 2020-11-21T10:31:07Z

@clalancette

Codes have been updated. Please help to review.
Besides, I have created PR (ros2/rosbag2#567) for rosbag2.

clalancette

This is looking great. I've got a few more minor things to fix up inline; once that is done, I'll go ahead and run CI. Thanks for all of the iterations here.

src/filesystem.c

include/rcutils/filesystem.h

clalancette · 2020-12-01T14:09:10Z

I did some minor cleanups in the test code just to avoid memory leaks if an assertion fails. You can see that work in 6b16702. With that, I'm happy with this code, so I'm going to approve and run CI.

clalancette · 2020-12-01T14:11:49Z

CI:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Barry Xu <[email protected]>

1. Revmoe header files for ssize_t 2. Update rcutils_calculate_directory_size() and related tests Signed-off-by: Barry Xu <[email protected]>

Signed-off-by: Barry Xu <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-12-01T20:28:13Z

So the Windows CMake warnings and test warnings can be ignored; those are in the nightlies (and at least some of them should be fixed for tomorrow).

The MSBuild ones I believe are fixed on master. So I just rebased this whole thing onto master and force-pushed it. I'll run another Windows CI to check:

Windows

clalancette · 2020-12-02T01:44:33Z

All right, Windows is now happier. One more run of the rest of the platforms, and I'll merge tomorrow:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-12-02T14:08:57Z

All right, going ahead and merging along with ros2/rosbag2#567. Thanks for the contribution @Barry-Xu-2018 .

fujitatomoya reviewed Oct 30, 2020

View reviewed changes

fujitatomoya approved these changes Nov 2, 2020

View reviewed changes

iuhilnehc-ynos reviewed Nov 2, 2020

View reviewed changes

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

clalancette reviewed Nov 6, 2020

View reviewed changes

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

src/filesystem.c Outdated Show resolved Hide resolved

Barry-Xu-2018 force-pushed the topic-enhance-rcutils_calculate_directory_size branch from b7c3412 to f517865 Compare November 12, 2020 06:18

clalancette requested changes Nov 13, 2020

View reviewed changes

fujitatomoya reviewed Nov 15, 2020

View reviewed changes

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

src/filesystem.c Show resolved Hide resolved

src/filesystem.c Outdated Show resolved Hide resolved

Barry-Xu-2018 force-pushed the topic-enhance-rcutils_calculate_directory_size branch from 39714fc to 767fd78 Compare November 16, 2020 07:34

clalancette requested changes Nov 16, 2020

View reviewed changes

src/filesystem.c Outdated Show resolved Hide resolved

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

clalancette requested changes Nov 19, 2020

View reviewed changes

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

clalancette requested changes Nov 20, 2020

View reviewed changes

Barry-Xu-2018 mentioned this pull request Nov 21, 2020

Update codes since rcutils_calculate_directory_size() is changed ros2/rosbag2#567

Merged

Barry-Xu-2018 mentioned this pull request Nov 22, 2020

Add leveldb storage plugin support ros2/rosbag2#556

Closed

Barry-Xu-2018 force-pushed the topic-enhance-rcutils_calculate_directory_size branch 2 times, most recently from c8fcc29 to 7d89863 Compare November 25, 2020 11:28

clalancette requested changes Nov 30, 2020

View reviewed changes

src/filesystem.c Outdated Show resolved Hide resolved

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

include/rcutils/filesystem.h Outdated Show resolved Hide resolved

clalancette approved these changes Dec 1, 2020

View reviewed changes

Barry-Xu-2018 added 3 commits December 1, 2020 20:26

Update rcutils_calculate_directory_size() to support recursion

affa177

Signed-off-by: Barry Xu <[email protected]>

Update codes based on review comments

e5ccde8

Signed-off-by: Barry Xu <[email protected]>

Update function note

119c459

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 and others added 8 commits December 1, 2020 20:26

Re-implement without consuming stack for recursion

2ebc782

Signed-off-by: Barry Xu <[email protected]>

Update based on review comments

b10dd2a

Signed-off-by: Barry Xu <[email protected]>

Update based on review comments

5f42f8b

Signed-off-by: Barry Xu <[email protected]>

Change size type from size_t to uint64_t

e1aff12

Signed-off-by: Barry Xu <[email protected]>

Update codes based on review comments

a825c0d

1. Revmoe header files for ssize_t 2. Update rcutils_calculate_directory_size() and related tests Signed-off-by: Barry Xu <[email protected]>

Fix a memory leak issue and change the new added test name

f8ec78b

Signed-off-by: Barry Xu <[email protected]>

Update function description and removing temporary variable

720f757

Signed-off-by: Barry Xu <[email protected]>

Small cleanups to avoid leaks on test failure.

bb4f1b1

Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the topic-enhance-rcutils_calculate_directory_size branch from 6b16702 to bb4f1b1 Compare December 1, 2020 20:26

clalancette merged commit e15e1d9 into ros2:master Dec 2, 2020

Barry-Xu-2018 mentioned this pull request Dec 3, 2020

Calculate the size of the specified directory (include subdirectory) #304

Closed

emersonknapp mentioned this pull request Feb 3, 2021

[foxy backport] Latest rosbag2 ros2/rosbag2#625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rcutils_calculate_directory_size() to support recursion #306

Update rcutils_calculate_directory_size() to support recursion #306

Barry-Xu-2018 commented Oct 29, 2020

Barry-Xu-2018 commented Oct 29, 2020

fujitatomoya commented Oct 30, 2020

fujitatomoya Oct 30, 2020

Barry-Xu-2018 Oct 30, 2020

fujitatomoya Oct 31, 2020

fujitatomoya left a comment

clalancette left a comment

Barry-Xu-2018 commented Nov 12, 2020

clalancette left a comment

Barry-Xu-2018 commented Nov 16, 2020

Barry-Xu-2018 commented Nov 16, 2020

clalancette left a comment

Barry-Xu-2018 commented Nov 19, 2020

clalancette left a comment

Barry-Xu-2018 commented Nov 20, 2020

clalancette Nov 20, 2020

Barry-Xu-2018 Nov 21, 2020

clalancette Nov 30, 2020

Barry-Xu-2018 Dec 1, 2020

clalancette Dec 1, 2020

Barry-Xu-2018 commented Nov 21, 2020

clalancette left a comment

clalancette commented Dec 1, 2020

clalancette commented Dec 1, 2020

clalancette commented Dec 1, 2020

clalancette commented Dec 2, 2020

clalancette commented Dec 2, 2020

		@@ -419,9 +419,9 @@ TEST_F(TestFilesystemFixture, calculate_directory_size) {
		#ifdef WIN32

		size_t
		rcutils_calculate_directory_size(const char * directory_path, rcutils_allocator_t allocator)

Update rcutils_calculate_directory_size() to support recursion #306

Update rcutils_calculate_directory_size() to support recursion #306

Conversation

Barry-Xu-2018 commented Oct 29, 2020

Barry-Xu-2018 commented Oct 29, 2020

fujitatomoya commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 12, 2020

clalancette left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 16, 2020

Barry-Xu-2018 commented Nov 16, 2020

clalancette left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 19, 2020

clalancette left a comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Nov 21, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Dec 1, 2020

clalancette commented Dec 1, 2020

clalancette commented Dec 1, 2020

clalancette commented Dec 2, 2020

clalancette commented Dec 2, 2020