search

Kitware / fletch

Computer Vision Software Development Environment
63 stars 54 forks source link

Patch PDAL for latest VS 2019 #646

Closed mleotta closed 3 years ago

dstoup commented 3 years ago

Jenkins test this please

dstoup commented 3 years ago

PDAL is getting really confused. The sudden appearance of this error suggests that libjsoncpp-dev got installed on the dashboard machine somehow. I removed it which should allow the Linux dashboard to pass, but there is something more fundamentally wrong here. PDAL is getting horribly confused about json, like several other packages. Most search for a header called json/json.h which is also provided by Fletch's libjson. When the library exists on the system and PDAL finds json/json.h things break because dimbuilder is using json/forwards.h, which does not exist. My first cut at a fix what to change their FindJSONCPP.cmake module to look for forwards.h instead of json.h since json.h is more common of a find under a json/ directory. But that fails because of other compiler errors. I still think that's probably a good fix. At least the find will search within the same package rather than getting more confused with Fletch in the mix as well. I can add that patch on a separate branch or we can add it here, which ever makes sense to you.

dstoup commented 3 years ago

Otherwise, LGTM.

mleotta commented 3 years ago

This change is specifically to fix compilation issues with the latest VS 2019. Something changed in the STL in the most recent VS 2019 that required comparison operators to be const. Some of PDAL's comparison operators were not marked as const, which was causing build errors. There is no reason why they shouldn't be const and this patch should not hurt anything else, so I think we can merge this separately from any other PDAL errors that may have cropped up on Linux.

dstoup commented 3 years ago

Sounds good. I will push the other patch on a new branch.

mleotta commented 3 years ago

@dstoup I can't actually figure out what the Windows failure is here. It looks like the Windows build actually was successful. but is reported as failed.

dstoup commented 3 years ago

@mleotta It's marked failing because of some kwiver tests that fail but I don't know that it's caused by this. I am going to verify the kwiver-check CI configuration and give this PR one more spin. I think it's probably ready to merge though, just want to be sure.

*Edit, especially not so concerned since (I don't think) anything in kwiver master uses PDAL.

dstoup commented 3 years ago

Actually, disregard my comment ... I found the build on open.cdash and what I see is normal. Merging.