BPCells icon indicating copy to clipboard operation
BPCells copied to clipboard

[cpp] Fix matrix Windows/linux matrix compatibility issues

Open immanuelazn opened this issue 7 months ago • 1 comments

Writing matrices on a Windows setup previously were unable to be read on Linux (and probably mac) setups, with the following error Error in eval(exprs, envir): Matrix has unrecognized version packed-double-matrix-v2.

This was a result of carriage lines in endlines created by Windows. Fix was required in two places, once in FileWriteBuilder::writeVersion() and in FileStringWriter::write(). Only needed to add std::ios_base::binary to use a binary stream instead. Doing so allowed for Windows setups to write with LF endings.

Nonetheless, we want to have backwards compatibility with previous BPCells matrices. in binaryfile.cpp, I rewrote the readLines() to also remove the carriage return via a conditional. I don't think this is a vectorizable for loop anyways, so having a conditional inside shouldn't be a problem.

For testing, I wanted to ensure that we could check for both a matrix generated on Windows before the fix with crlf endings, and after the fix. I could tarball these if desired as well.

Fixes #253

immanuelazn avatar May 13 '25 03:05 immanuelazn

I noticed that git messes with the CRLF endings by default which makes the test files a bit tricky to preserve line endings on. I added additional checks in the tests to make sure the test files themselves are as expected, and added a .gitattributes file that should help.

There seems to be a little weirdness about whether git respects the settings from .gitattributes when the settings exist only in a branch. This stack overflow answer pointed me towards running git clone --branch ia/windows-string-writer-fix in a fresh folder, which appears to keep the crlf endings preserved.

Other than that your fixes worked perfectly for me once I got a windows VM up and running, so my changes are mostly just making sure that our test files are solid.

bnprks avatar May 16 '25 00:05 bnprks

Good catch on the git issues. I didn't know that git automatically changes line return when pulling, and was probably missed because I was only developing on one system. Otherwise, the changes look good. I will merge this in.

immanuelazn avatar May 19 '25 19:05 immanuelazn