asmaloney / libE57Format

Library for reading & writing the E57 file format
Boost Software License 1.0
136 stars 64 forks source link

Tests failing on CLion using MinGW GCC #245

Open UshnaGul opened 1 year ago

UshnaGul commented 1 year ago

I am experiencing issues with the tests failing on CLion using the MingW GCC compiler. Specifically, the issue seems to be caused by the readPhysicalPage method in CheckedFile.cpp throwing an exception due to the result not being equal to the physical page size. When I comment out this exception, the tests run fine, but then I encounter issues with checksum calculation.

Operating system: Windows 11 (64-bit) Compiler version: GCC IDE: CLion Please let me know if there is any additional information that you need.

asmaloney commented 1 year ago

Yet another issue in CheckedFile - I'm not surprised 😄

1) Can you add the stack trace so we know where it's coming from? 2) Which test is it failing on (or is it all of them)? 3) My guess is that we are using _WIN32 somewhere where we should probably be using _MSC_VER (or vice-versa). 4) I don't have any way to debug this since I don't use Windows/MinGW, so someone else will probably have to solve it and submit a PR.

UshnaGul commented 1 year ago

Thanks for your quick response. Here's the stack trace that I'm seeing when the readPhysicalPage method throws the exception: ImageFileImpl() called, fileName=./no-path/empty.e57 mode=w passed /n ImageFileImpl() called, fileName=./empty.e57 mode=w StructureNodeImpl::set(pathName=formatName, ni, autoPathCreate=0 pathNameParse pathname=formatName pathNameParse returning: isRelative=1 fields.size()=1 fields=formatName, StructureNodeImpl::set: level=0 field[0]: formatName StructureNodeImpl::set(pathName=guid, ni, autoPathCreate=0 pathNameParse pathname=guid pathNameParse returning: isRelative=1 fields.size()=1 fields=guid, StructureNodeImpl::set: level=0 field[0]: guid StructureNodeImpl::set(pathName=versionMajor, ni, autoPathCreate=0 pathNameParse pathname=versionMajor pathNameParse returning: isRelative=1 fields.size()=1 fields=versionMajor, StructureNodeImpl::set: level=0 field[0]: versionMajor StructureNodeImpl::set(pathName=versionMinor, ni, autoPathCreate=0 pathNameParse pathname=versionMinor pathNameParse returning: isRelative=1 fields.size()=1 fields=versionMinor, StructureNodeImpl::set: level=0 field[0]: versionMinor StructureNodeImpl::set(pathName=e57LibraryVersion, ni, autoPathCreate=0 pathNameParse pathname=e57LibraryVersion pathNameParse returning: isRelative=1 fields.size()=1 fields=e57LibraryVersion, StructureNodeImpl::set: level=0 field[0]: e57LibraryVersion StructureNodeImpl::set(pathName=data3D, ni, autoPathCreate=0 pathNameParse pathname=data3D pathNameParse returning: isRelative=1 fields.size()=1 fields=data3D, StructureNodeImpl::set: level=0 field[0]: data3D StructureNodeImpl::set(pathName=images2D, ni, autoPathCreate=0 pathNameParse pathname=images2D pathNameParse returning: isRelative=1 fields.size()=1 fields=images2D, StructureNodeImpl::set: level=0 field[0]: images2D terminate called after throwing an instance of 'e57::E57Exception' what(): E57 exception Process finished with exit code 3 Error Read Failed: fileName=./empty.e57 result=1023/n image

The read/write tests fail (6 of 26 tests passed) due to the readPhysicalPage method, and when I comment out the exception, the tests pass but I encounter issues with checksum calculation.

I'm not sure if _WIN32 or _MSC_VER is being used somewhere where it shouldn't be, but I'll take a look at the code to see if I can find anything. If anyone else has any ideas or suggestions, please let me know.

Thanks again for your help.

asmaloney commented 1 year ago

I would look at CheckedFile::lseek64 which is called (eventually) by this line:

      const uint64_t physicalLength = length( Physical );

That #ifdef section around line 488 is one of the potential _WIN32/_MSC_VER places I'm talking about. I wonder if it's using the wrong seek function?

My guess is that that first check should be for MSVC rather than Windows and that we need to add or adjust one of the cases there to handle MinGW somehow.

I don't know what _LARGEFILE64_SOURCE or __LARGE64_FILES are about at the top, but that might also be related?

UshnaGul commented 1 year ago

https://github.com/d-bahr/CRCpp/issues/17

Could the issue be related to this? My CMake version is 3.25.2 The warnings are ignored when I run my project which includes the library but when I build the library separately, it errors out due to the following in CompilerWarnings.cmake: function( set_warning_as_error ) message( STATUS "[${PROJECT_NAME}] Treating warnings as errors")

if ( CMAKE_VERSION VERSION_GREATER_EQUAL "3.24" )
    set_target_properties( ${PROJECT_NAME}
        PROPERTIES
            COMPILE_WARNING_AS_ERROR ON
    )
else()
    # This will do nothing on compilers other than MSVC, Clang, Apple Clang, and GNU compilers.
    target_compile_options( ${PROJECT_NAME}
        PRIVATE
            $<${compiler_is_msvc}:
                /WX
            >
            $<$<OR:${compiler_is_clang},${compiler_is_gnu}>:
                -Werror
            >
    )
endif()

endfunction()

asmaloney commented 1 year ago

Could the issue be related to this?

Well not with that CMake code, but maybe with the warnings it's treating as errors. That CMake code just turns on errors when it builds itself to help catch compatibility issues between platforms/compilers.

What warnings/errors are being produced when you compile the library on its own (or turn on E57FORMAT_WARNING_AS_ERROR in your project)?

UshnaGul commented 1 year ago

image

asmaloney commented 1 year ago

OK - that's a separate issue. I created a new one for it.