Disable apparent-size in du command of get_size_on_disk

[GeigerJ2]

In #6584 the get_size_on_disk method was added to RemoteData, which calls the du utility as the default way to obtain the size of an associated file/directory, using the -s (summary) and --bytes option. As discovered in #6696, however, the relevant tests started failing when the ubuntu-latest environment of GHA got updated to point to Ubuntu 24.04 rather than 22.04.

As it turns out, --bytes does not only give the output in bytes, but is actually equivalent to --apparent-size --block-size=1. I'm not sure what exactly has changed between the different Ubuntu versions, if it's the behavior of the --apparent-size flag of du, or the way the apparent size is determined by the file system. Nonetheless, using apparent size makes the output fragile, as seen from the failing tests. Thus, I now changed the command from du -s --bytes to du -s --block-size=1. This also leads to the obtained file sizes for the different test cases to be larger (as expected), so these are also updated.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.99%. Comparing base (15b5caf) to head (cf9b884).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6702      +/-   ##
==========================================
+ Coverage   77.99%   77.99%   +0.01%     
==========================================
  Files         563      563              
  Lines       41761    41762       +1     
==========================================
+ Hits        32567    32570       +3     
+ Misses       9194     9192       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[unkcpz]

Thanks, I assume the different behavior can be tracked down to following change in coreutils, see https://fossies.org/linux/coreutils/ChangeLog

 3377 2023-03-13  Pádraig Brady  <[email protected]>
 3378 
 3379 	tests: adjust du test for recent apparent size change
 3380 	* tests/du/threshold.sh: Directories are assumed to be
 3381 	of size 0 with --apparent since commit v9.1-187-g110bcd283
 3382 	so remove --apparent cases from this test.

It seems the --apparent-size give a more realistic size of file you created rather than the block size that can depend on the FS. But not sure how to use the combination of options to make the output consistent between different du version.

[danielhollas]

Thanks for figuring this out!
Just one ask for an explanatory comment in the code (i.e. basically what you write in a PR description).

[GeigerJ2]

Interesting, thanks for digging this up, @unkcpz!

It seems the --apparent-size give a more realistic size of file you created rather than the block size that can depend on the FS. But not sure how to use the combination of options to make the output consistent between different du version.

Yeah, the apparent size is more realistic in that sense, as it only captures the actual data. Nonetheless, on your file system, even a file with less content will occupy the entire block, so I feel like it's not really that useful in the end (the other option, stat does actually give the apparent size). Now, the issue is still that block sizes might differ, though, 4096 bytes should be the default for Linux (ext4), and also macOS, so possibly it's fine to just go with that?

[danielhollas]

Nonetheless, on your file system, even a file with less content will occupy the entire block, so I feel like it's not really that useful in the end.
I agree, ultimately we want to use options that are useful for the users, and not based in on the test suite. The users of --get_size_on_disk are interested in the actual space that things take right? And probably they are interested the largest files anyway, where the difference should be smaller.

If the tests can't be stabilized, perhaps then we need to modify the tests, not the implementation...

[unkcpz]

Nonetheless, on your file system, even a file with less content will occupy the entire block, so I feel like it's not really that useful in the end (the other option, stat does actually give the apparent size). Now, the issue is still that block sizes might differ, though, 4096 bytes should be the default for Linux (ext4), and also macOS, so possibly it's fine to just go with that?

From users' point of view, I think using just the block size as current implementation can be better. The actual size may not matter too much but the size transfer over internet (by buffer size I think usually the default is 4kb) or the size occupy on the disk which mostly are computed by block. Thus I len to the changes of this PR.

[unkcpz]

Just a minor request on docstring, otherwise all good for me.

[GeigerJ2]

If the tests can't be stabilized, perhaps then we need to modify the tests, not the implementation...

@danielhollas I'd say the actual problem was the implementation :D But this whole file size business is pretty annoying. We spent quite some time discussing this in the office already.

Anything else to add from your side?

[danielhollas]

Looks great, thank you for the extensive comment! Just one quip about the double meaning of "block size", but not a blocker.

[danielhollas]

Ah, I see that the tests are already failing for the last commit on main so this should just go in I think.