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

static build #21

Merged
merged 4 commits into from
Mar 21, 2022
Merged

static build #21

merged 4 commits into from
Mar 21, 2022

Conversation

gauteh
Copy link
Contributor

@gauteh gauteh commented Sep 3, 2021

Add static feature flag as discussed in #20. Builds and tests fine. libgeos version is 3.9.1.

To build make sure that submodules are updated and inited: git submodule update --init.

❯ ldd libgeos_sys.so
        linux-vdso.so.1 (0x00007fff3c773000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fc515d37000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fc515b1f000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fc515917000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fc5156f8000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fc51535a000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fc515156000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fc514d65000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fc516470000)

Motivation

  • Allows dependent crates to be built into python packages supporting the manywheels spec (making them easier to distribute)
  • Allows easier compilation of geos-sys without needing libgeos installed on the system
  • Allows easier distribution of binaries / libs without needing same version of libgeos
  • Can use a specific / and same version of libgeos

@gauteh gauteh marked this pull request as draft September 3, 2021 19:03
@gauteh gauteh marked this pull request as ready for review September 3, 2021 19:04
@gauteh
Copy link
Contributor Author

gauteh commented Sep 3, 2021

Some licencing stuff missing. Currently pinned to geos 3.9.1.

@gauteh
Copy link
Contributor Author

gauteh commented Sep 6, 2021

I updated the PR with some motivation (top entry).

@gauteh
Copy link
Contributor Author

gauteh commented Sep 6, 2021

There is something wrong with this still, I haven't been able to correctly link geos.

@gauteh
Copy link
Contributor Author

gauteh commented Sep 6, 2021

Figured it out. Had forgotten to link to geos C++ library too.

@gauteh
Copy link
Contributor Author

gauteh commented Sep 10, 2021

geos tests pass with this one as well as tests using a library dependent on geos again. It is an open question which version of geos to ship.

@gauteh
Copy link
Contributor Author

gauteh commented Nov 27, 2021

Hi. This has been tested extensively for a good while now. This change enables the use og this crate in manylinux python packages.

@pka
Copy link
Member

pka commented Nov 29, 2021

I'm very excited about the possibility to use GEOS in Rust without external dependency! Code looks fine for me. Only question is about embedding the C source code with a git submodule. Does a shallow clone still work, resp. is cloning the submodule only necessary for static linking?

@GuillaumeGomez: Would be great having that merged!

@gauteh
Copy link
Contributor Author

gauteh commented Nov 29, 2021 via email

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

Hi, do you have any chance at looking at this? Been using for a long time.

@GuillaumeGomez
Copy link
Member

Sorry, I was busy when you opened this PR and then completely forgot about it. Sorry about that. Thanks for this improvement! I'll merge as is to not take any more time and then add a mention of this feature in the README.

@GuillaumeGomez GuillaumeGomez merged commit c487c1c into georust:master Mar 21, 2022
@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

Thanks a lot! This should make georust/geos#99 work as well (perhaps after a release).

@GuillaumeGomez
Copy link
Member

I'm just curious about something: isn't it possible to have this static link without creating a new crate?

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

Hm, maybe, I think you'd have to (optionally) build the -src crate in build.rs and link it in there.

@GuillaumeGomez
Copy link
Member

That'd make things much easier overall. Want to give it a try?

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

I just tried a little, and it would probably involve calling cargo from build.rs. I see that other libraries usually distribute a -src package:

I think that it would be a bit fragile and messy, so I think the best solution is a -src crate, even though that is not super-great. It does open for depending on different -src versions, which would help the end-user (I have sometimes, temporarily, needed to depend on a specific -src version of openssl to get the build to work).

@GuillaumeGomez
Copy link
Member

Then it's fine. Let's keep thins as is. Just one last thing: what happens if the submodule isn't present when you build in the repository? By default, when building a dependency, I guess it won't be present so it's very likely that it'll never work, right?

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

Without static you won't notice. With static it won't work and you'll have to init them before building. Note that cargo checks out with submodules when installing as deps or binaries.

@GuillaumeGomez
Copy link
Member

Does it when you simply use it as a dependency?

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022

If you use it as a simple dependency (e.g. https://github.com/gauteh/roaring-landmask/blob/main/Cargo.toml#L21) then you don't have to think about it. It will be checked out as necessary by cargo. So both static and without will work without knowing about the git repo structure. I am not sure if the submodule is mirrored to crates.io as well, but it works for all other packages I've used (netcdf, hdf, openssl). Pretty sure I modelled this setup after openssl.

@GuillaumeGomez
Copy link
Member

Then it's fine. I'll publish both crates so you can update the geos PR.

@gauteh
Copy link
Contributor Author

gauteh commented Mar 21, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants