@emersonknapp - please run this CI job
Gist: https://gist.githubusercontent.com/prajakta-gokhale/211bac73533a4576537436f416baea2a/raw/d03c798754795f5e307bdf4d7922654491587bca/ros2.repos
BUILD args: --packages-up-to rosbag2_transport
TEST args: --packages-select rosbag2_transport
Job: ci_launcher

I wonder if we should just disable this test for 32 bit platform. It doesn't feel correct to say that these maxed out values are actually a tebi/pebibyte in the first place. Thoughts?

I agree with that part, it is very odd to say this.

But actually, this is problematic in other ways. First of all, this should have nothing to do with address space size; this is formatting for a file size, and there is no reason you can't have a 1 TiB file on 32-bit armhf.

The real problem looks to be in the API. It is using a size_t for the number passed into format_file_size, which is only 32-bit on armhf, so it can only represent up to 4GiB. I think the correct fix here is to switch to a uint64_t for the format_file_size API, which means that all of this will work on both 32-bit and 64-bit platforms.

The real problem looks to be in the API. It is using a size_t for the number passed into format_file_size, which is only 32-bit on armhf, so it can only represent up to 4GiB. I think the correct fix here is to switch to a uint64_t for the format_file_size API, which means that all of this will work on both 32-bit and 64-bit platforms.

I didn't realize that, thanks for pointing it out! Will change it in the next commit.

• Linux
• Linux-aarch64
• macOS
• Windows
• Linux-armhf

@clalancette please take a look agian, thanks!

@Karsten1987 could you please review these changes and let us know what you think about changing member variable types from size_t to uint64_t?

I agree, having size_t here is indeed prone to error. Therefore, I agree with @clalancette for the proposed changes. I am still a bit undecided whether we have to bump the version when it's obviously a wrong behavior.

I am still a bit undecided whether we have to bump the version when it's obviously a wrong behavior.

So for the bag_size itself, there is actually a comment there saying that it is not serialized. If we just change that, we may not have to bump the version. But the fact that message_count is also a size_t, and is serialized, means (to me) that we have to bump the version. There probably aren't a ton of bag files out there now, but having a different size without bumping the version may lead to difficult-to-debug bugs.

There probably aren't a ton of bag files out there now, but having a different size without bumping the version may lead to difficult-to-debug bugs.

I agree with that reasoning. For the sake of fixing unit test failures in the nightlies, we could merge this PR as is and create a new one to change variable types (and bump up the version?)

I agree with that reasoning. For the sake of fixing unit test failures in the nightlies, we could merge this PR as is and create a new one to change variable types (and bump up the version?)

I'd prefer not to do that; it will be too easy to forget about this problem once it is "hidden" with this PR merged.

@prajakta-gokhale Please make the changes to BagMetadata switching the size_t -> uint64_t, and bumping the version. @Karsten1987 can then review, and if he still disagrees with the version bump it's just a one-line change.

@Karsten1987 I have changed the variable types and bumped up BagMetadata::version in the latest commit. Please take a look.

Lets also check that the armhf tests are fixed :)

@Karsten1987 we're happy with this - if you are too let's get this merged - I think this is the final PR for ci_linux-armhf to be fully green.