-
Notifications
You must be signed in to change notification settings - Fork 46
Make libproj shared and enable pyproj [full build] #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Package Build ResultsTotal packages built: 56 Package Build Times (click to expand)
Longest build: libgdal (29m 13s) |
Pretty sure we don't need to build the port, less sure we don't need the python package for some reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so do you think the problem is that sqlite3 is linked both in libproj and pyproj?
Then what happens if we just keep libproj static and remove sqlite3 from pyproj?
Anyway, I am happy if this solves the problem. I don't fully understand why... but there are always unknown unknowns in package build, so if it works, I am happy to merge it. I triggered full build (building all packages) to see if there is any regression. |
Not quite, I think it's that pyproj has 10 different extension modules (like Though I'm not actually sure what the consequences of linking sqlite in libproj and then again in pyproj are 🤔 I guess the 10 extension modules could then also each have had some conflicting sqlite stuff inside? But I haven't personally seen any sign of that in what I've looked at so far. |
I can give this a quick try though, and revert if it fails (as I think it will) |
I think this is a place my lack of knowledge about compilers is showing - keeping the static build of libproj and removing the link flag in pyproj causes sqlite not to be included at all. Testing locally, I get Whereas if libproj is a shared library build, I suppose it gets a link step, so sqlite3 is included at that point. I'll let the CI finish now it's started, but I think the |
locally, vs
on CI Not having PyInit seems a bit extreme, I can't really explain that. |
Hmm okay, I did want to use rasterio, I'll look into it. It created an issue with libgdal I think |
I see. I don't understand why it worked before and why it doesn't anymore, but I am fine with adopting your original approach if other packages work fine. Thanks for the investigation! |
I think you need to add libproj as a run dependency of libgdal. As libproj is now shared library, libproj need to be loaded runtime when loading libgdal. |
Same, it's still a mystery to me. The only thing I have to go on is that from PROJ 9.3.1 to 9.4.0, they changed from a custom FindSqlite3 to the default cmake one. But I have no idea why that would affect anything. Maybe something changed in sqlite itself as well. I'm getting round to this, probably tomorrow. |
Can't reproduce the issue locally so a bit suspicious of this
Same issue :/ Locally, it seems to even work statically linking libproj (to libgdal, not pyproj), I guess because the .a is still created |
See this discussion pyodide/pyodide#5887
TLDR: different instantiations of sqlite in different pyproj extension modules meant that sqlite was initialising global function pointers in one module and failing to find them in another. At least this is my take. Making libproj shared (and by extension, the statically linked sqlite inside libproj) appears to solve the issue.
This PR is experimental for now until CI passes and I've thought about it a little more.Update: I bumped Cartopy and geopandas to check they pass too and looks good from here.
I can't see (as a new contributor) any more issues to solve. Apologies if I've missed something, let me know and I can change it.