Mac build issue spotted in 3.8.0 Rec model

[davemfish]

These two logs were sent to me. I don't know what's going on here, but maybe it's an issue with a library shipped with the Mac distribution?

The Problem Report includes:
Assertion failed: (0), function query, file AbstractSTRtree.cpp, line 293.

ProblemReport_14_44_46.txt
InVEST-Recreation-Model-log-2020-04-08--14_44_46.txt

[phargogh]

@davemfish I'm wondering if this is the same issue at shapely/shapely#260. If it is, could you check the import order as mentioned? On mac, anyways, there's some weirdness around which version of the GEOS DLL is loaded, and the shapely/GDAL import order seems to matter.

[davemfish]

Oh hey, that's you! Indeed, the osgeo imports are ahead of the shapely imports. So presumeably swapping them should fix this.

@phargogh do you think it's a good idea for us to add MacOS to our test suite? If so I can put that on my list and test out this fix.

[phargogh]

Ha :) yeah, that issue was a real head-scratcher for a while there! Hopefully switching the imports will address the issue here, and yes, adding tests for mac would be a very good idea as well. Thanks for taking a look @davemfish !

[davemfish]

resolved in #92

[davemfish]

Apparently this is still an issue in 3.8.2
InVEST-Recreation-Model-log-2020-05-21--09_58_11.txt
MacProblemReport_09_58_11.txt

It looks like the fix (swapping the imports) was never made (my bad) when this ballooned into the larger issue of adding MacOS tests.

[davemfish]

Sama - who reported this issue both times - tried a build with the imports swapped and reported the same crash.

[phargogh]

From fiddling with things locally, the issue looked to be related to the way that dylib files are included in the InVEST pyinstaller-generated application folder. The GEOS libraries were located at:

lib/libgeos_c-3.8.0.dylib
lib/libgeos_c.1.dylib

When in actuality what shapely.geos tries to import is called libgeos_c.dylib. Since it can't be found in the CWD relative to the invest binary itself, the files need to be put there. A modified invest.spec now includes these files:

lib/libgeos_c-3.8.0.dylib
lib/libgeos_c.1.dylib
lib/libgeos_c.dylib
libgeos_c-3.8.0.dylib
libgeos_c.1.dylib
libgeos_c.dylib

Perhaps redundant, but the application seems to load. We can take a look at the redundancy once we've confirmed that the application resolves the apparent conflict with homebrew (related to #10).

[phargogh]

@davemfish would you happen to know if the dev build for Sama addressed the issue?

[davemfish]

Ah yes, thanks for the ping. That build did fix the issue, says Sama.

[dcdenu4]

@davemfish @phargogh I'm currently seeing an issue with a mac binary build that seems to have some of the same issues discussed here:

https://github.com/dcdenu4/invest/runs/1276850816

For context I have bumped Shapely>=1.7.0 and GDAL==3.1.2.

@phargogh you mention:

A modified invest.spec now includes these files:
lib/libgeos_c-3.8.0.dylib
lib/libgeos_c.1.dylib
lib/libgeos_c.dylib
libgeos_c-3.8.0.dylib
libgeos_c.1.dylib
libgeos_c.dylib

but I don't see that reflected in invest.spec.

[phargogh]

@dcdenu4 ha, I had this on a branch that I hadn't touched in a while! Here's the commit I was thinking about: phargogh@0ac3f4b

This ended up addressing #10 as well, since some of those DLLs weren't being packaged up by pyinstaller into the mac binary, so then the computer would look for them from the broader environment and, if you had a version mismatch of a library, often causing an error and preventing the application from launching.

[dcdenu4]

@phargogh ahhh the forgotten branch! I haven't seen any issues with proj lately but we did just add in the shapely dylibs in #350 . Although your comments and itertools.chain were a bit more robust then what I put in there.

So, should we close this issue then?

[phargogh]

I'll take a closer look at that issue and maybe submit a new PR for that change. Being careful to always include those extra dylibs would really help to avoid the DLL version conflicts described in #10!

[phargogh]

Closing now that these changes are included in release/3.9 after #358 was merged. Feel free to reopen if we need to!