Update to assimp 5.2.2

[cottsay]

This matches the version distributed in Ubuntu Jammy: https://packages.ubuntu.com/jammy/libassimp-dev

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[Yadunund]

LGTM

[cottsay]

While this compiles just fine, I'd like to hear from someone on the RViz team about how this package is used in RViz and how we could verify that things still work as expected.

[cottsay]

I've found one issue with this so far. It appears that if an unacceptable version of assimp is installed, the cmake config for that version might be used and successfully discover the newer version that is being vendored, but may not match the configuration that was built in the vendor package, which will cause the library to be assimp::assimp-NOTFOUND.

Uninstalling the system package resolves the issue, but that's not a solution. Still working on that.

[Yadunund]

Removed system libassimp5 and built ros2 workspace with -DFORCE_BUILD_VENDOR_PKG=On

Loaded a few models which have a variation of stl, obj and dae meshes for their links and everything looked ok 👍🏼

[cottsay]

Alright, this is blocked on #970. Without that change, downstream builds will fail on platforms which have a system assimp package which defines a CMake target but is less than 5.2.0.

[ahcorde]

conflicts

[clalancette]

This looks good to me with green CI.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[cottsay]

@ros-pull-request-builder retest this please

[clalancette]

I think there are two remaining issues with this PR:

1. The patches are failing to apply on Windows. That probably has something to do with line endings or something else fun.
2. I think we should probably remove all of the ASSIMP_CXX_FLAGS when building on GCC/Clang except for -std=c++14. In my testing locally, all of the warnings there are actually fixed in the code in assimp 5.2.2, so there is no need for us to pass them anymore.

[codebot]

Looks like line 13 (an empty line) in the patch file has a space, but in the upstream code there is no space on that line. Apparently git apply is not always OK with that in a context line. It's interesting that it applies fine on Linux, but fails on Windows.

For reference, this is the upstream line:
https://github.com/assimp/assimp/blob/v5.2.2/code/Common/ZipArchiveIOSystem.cpp#L197C1-L197C1

I couldn't see the difference until downloading each file and looking in a hex editor to be certain; it seems spaces are sometimes not visible in the GitHub UI.

[clalancette]

Looks like line 13 (an empty line) in the patch file has a space, but in the upstream code there is no space on that line. Apparently git apply is not always OK with that in a context line. It's interesting that it applies fine on Linux, but fails on Windows.

Actually, I don't think that is the issue. At least, in the testing I did locally, removing that space didn't make a difference. I think the problem is more subtle.

In particular, it looks like Git will automatically convert \n to \r\n when it is checking out text files. And the patch files are considered text files. So it means that Git helpfully corrupts the patch when the clone is happening.

The good news is that there seems to be a workaround by adding a .gitattributes file to the repository. That seemed to fix the issue for me locally. I'm going to add that here and see if it makes a difference to CI.

[codebot]

Yes indeed! Line endings strike again. I just made a PR to this PR that adds the .gitattributes file to ensure that the patch file line endings match up with upstream line endings on Windows checkouts. It turns out that upstream declares eol=lf on all its files, so on Windows they have LF (unix) line endings, whereas our patch file checks out on Windows with CRLF endings. #1004 sets eol=lf to our patch file, so the line endings match on Windows checkouts. This seems to work as expected now in isolated local testing.

[clalancette]

All right, with all of those fixes now in, let's try another CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

Sigh. There is one more warning on Windows:

CUSTOMBUILD : CMake warning :  [C:\ci\ws\build\rviz_assimp_vendor\assimp_vendor.vcxproj]

I have no idea what that means, but we'll need to fix that as well.

[clalancette]

After (hopefully) figuring out the Windows issue, here is another try for CI:

• Linux
• Linux-aarch64
• Windows

[cottsay]

RPM distros use lib64 - I'd like to see RHEL CI on this change since you removed that line.

[clalancette]

RPM distros use lib64 - I'd like to see RHEL CI on this change since you removed that line.

Yeah, good point. If the previous CI was successful I was going to run more comprehensive CI, but I'll try this locally on RHEL first and see what happens.

[clalancette]

RPM distros use lib64 - I'd like to see RHEL CI on this change since you removed that line.

And you were right about this; that line is needed for RHEL. I've added it back conditionally, only on !WIN32. And that now all looks good in my local testing. I'm going to run a bunch of CI here.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows
• Windows Debug
• RHEL
• Clang
• Linux (FORCE_BUILD_VENDOR_PKG=ON)

Stale review.

[clalancette]

All right, everything here is green. This update was a bit more...delicate than I would have liked, but we seemed to have successfully danced our way around the issues. Going ahead and merging.