Skip to content
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

geo: WKT writing not working with GEOS bundled with CockroachDB #54841

Closed
timgraham opened this issue Sep 26, 2020 · 9 comments
Closed

geo: WKT writing not working with GEOS bundled with CockroachDB #54841

timgraham opened this issue Sep 26, 2020 · 9 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner

Comments

@timgraham
Copy link
Contributor

timgraham commented Sep 26, 2020

Describe the problem

Django's WKTWriter.write() doesn't work with the GEOS library bundled with CockroachDB.

To Reproduce

> from django.contrib.gis.geos import GEOSGeometry, WKTWriter
> wkt_w = WKTWriter()
> ref = GEOSGeometry('POINT (5 23)')
> wkt_w.write(ref).decode())
POINT ( )

Expected behavior

Ouput of: POINT (5.0000000000000000 23.0000000000000000).

The expected behavior happens with GEOS 3.8.1 compiled from source but not with the GEOS 3.8.1 included with CockroachDB.

Additional information

Here's Django's interface to GEOSWKTWriter_write.

If no one has any idea what the cause may be, I can investigate further. I'm not intimately familiar with how this all works.

Environment:

  • CockroachDB version 20.2 beta 2

Jira issue: CRDB-3710

@blathers-crl
Copy link

blathers-crl bot commented Sep 26, 2020

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

  • @rafiss (found keywords: Django)
  • @cockroachdb/bulk-io (found keywords: import)

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Sep 26, 2020
@timgraham timgraham changed the title WKT writing not working with GEOS bundled with CockroachDB geo: WKT writing not working with GEOS bundled with CockroachDB Sep 27, 2020
@otan
Copy link
Contributor

otan commented Sep 28, 2020

the WKT unit tests work, so I'm not sure how this came about.

@timgraham
Copy link
Contributor Author

Could you point to how libgeos.so and libgeos_c.so are generated? Maybe inspecting that would reveal something. I've been looking more closely at the Python side but I haven't come across anything suspicious.

Incidentally, is it possible to make it so that patchelf --set-rpath '/usr/local/lib/cockroach/' /usr/local/lib/cockroach/libgeos_c.so isn't required? Also sudo ln -s /usr/local/lib/cockroach/libgeos.so /usr/local/lib/cockroach/libgeos.so.3.8.1? (For anyone else reading, we discussed these things at #54429 (comment).)

@otan
Copy link
Contributor

otan commented Oct 1, 2020

Could you point to how libgeos.so and libgeos_c.so are generated? Maybe inspecting that would reveal something. I've been looking more closely at the Python side but I haven't come across anything suspicious.

https://github.com/cockroachdb/cockroach/blob/master/Makefile#L798

You can try fudge with this if you are willing -- ./build/builder.sh mkrelease linux will build the binary and libs will be in lib.docker_amd64/

Incidentally, is it possible to make it so that patchelf --set-rpath '/usr/local/lib/cockroach/' /usr/local/lib/cockroach/libgeos_c.so isn't required? Also sudo ln -s /usr/local/lib/cockroach/libgeos.so /usr/local/lib/cockroach/libgeos.so.3.8.1? (For anyone else reading, we discussed these things at #54429 (comment).)

I took a look at this and it was quite complicated with our build setup, and we're revamping that in the upcoming release anyway (the issue in particular is that cmake and specifying rpath does not work when cross compiling).

Note we already special case mac because of this, but the patchelf tool was not available in the builder.

@otan
Copy link
Contributor

otan commented Oct 1, 2020

does

+       patchelf --set-rpath '/usr/local/lib/cockroach/' lib/libgeos_c.so
+       patchelf --set-soname libgeos.so lib/libgeos.so
+       patchelf --replace-needed libgeos.so.3.8.1 libgeos.so lib/libgeos_c.so

avoid the need for the symlink?
note that this patchelf isn't required if you dlopen libgeos_c.so before libgeos_c.so.

@timgraham
Copy link
Contributor Author

Yes. I ran those commands from /usr/local/lib/cockroach/ and libgeos_c.so is in that directory so I omitted lib/.

@timgraham
Copy link
Contributor Author

I ruled out issues with the Cockroach GEOS fork by compiling it from source. All Django tests pass with that libgeos_c.so.

Perhaps it's best to wait and see if the revamp to the build process that you mentioned happens to fix this issue.

craig bot pushed a commit that referenced this issue Oct 2, 2020
55129: build: correct ELF info for libgeos r=petermattis a=otan

Looks like cross compiling with rpath does not work, so install patchelf
and use them on the given .so files such that dlopen works using the
regular pattern.

Refs #54841

Release note (general change, bug fix): Fixed the rpath and so names of
libgeos.so and libgeos_c.so such that a dlopen to libgeos.so is not
needed.



Co-authored-by: Oliver Tan <[email protected]>
@timgraham
Copy link
Contributor Author

I confirmed Django works without creating a symlink and modifying the libgeos files in Cockroach 21.1 beta 1. The strange WKT writing issues described here still remain.

@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
@timgraham
Copy link
Contributor Author

This started working in v22.1 alpha 4. libgeos_c.so and libgeos.so there are different than those in the alpha 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

No branches or pull requests

4 participants