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

Request: Option to not require external dependencies for static linking scenarios #8

Closed
socmag opened this issue Feb 19, 2018 · 13 comments
Assignees

Comments

@socmag
Copy link

socmag commented Feb 19, 2018

Hi, great to see the new version of S2 out - very cool!

I know a lot of people use the old version of S2 (us for example) and link it statically into their application. That was fairly easy to do with the old version, but now with the GFlags, GLog dependencies it's a lot more onerous and also affects downstream build system requirements.

Is there any chance you might consider an option to allow building the core without requiring any dependencies, aside from libssl of course.

Best

@jmr
Copy link
Member

jmr commented Feb 20, 2018

I'm hoping that flags and logging move into Abseil. Then we can either have zero or one dependency, depending on whether we include our own Abseil version.

@socmag
Copy link
Author

socmag commented Feb 22, 2018

hmmm okay, that sounds like a long term thing?

sounds like it might be easier to fork it for now.

@graetzer
Copy link
Contributor

I don't want to annoy you guys, but I would also second the idea of making glog optional / internal.

Especially the glog/logging.h header contains some macros like CHECK which collides with macros from other libraries. Maybe you could instead use basic asserts in header files?
For example this makes it a little hard to include s2geometry headers in tests written with Catch

@socmag
Copy link
Author

socmag commented Feb 22, 2018

I've thrown together a quick fork that builds the static lib.
Also updated to be aligned with latest libssl BIGNUM

Hope it is useful in the interim
Standalone S2Geometry

Best!

@socmag
Copy link
Author

socmag commented Feb 22, 2018

Especially the glog/logging.h header contains some macros like CHECK which collides with macros from other libraries.

@graetzer I just prefixed those macros with S2_ in the fork, hopefully that will help. I had the same problem.

@jmr
Copy link
Member

jmr commented Feb 23, 2018

I'm not thrilled by the idea of reimplementing subsets of glog and gflags to include. Would including these as git submodules make things better for you?

A few notes about your standalone version:

  1. There may be a performance penalty for using BN_new instead of BN_init.
  2. Some tests need FlagSaver, which doesn't exist in your gflags.h
  3. Behavior of CHECK and LOG(FATAL) is different. Check failures and FATAL currently abort.

@dhly-etc
Copy link
Contributor

@jmr The way I hope to use the S2 library is as a statically embedded library within a product which has its own logging and CLI argument parsing facilities. Including the glog and gflags dependencies introduces a problem not just because it introduces more dependencies, but because of conflicts with other components of the higher-level application.

So what I would personally like (and I think what @socmag and @graetzer were trying to get at), is for the logging and CLI configuration to be optional. So for instance, change the code so that if the glog library is found at compile time, enable logging; otherwise disable logging altogether. For applications which manage their own logging, this is sufficient: the S2 library methods return enough information to do error handling robustly and let the application decide if it wants to log something itself. Similarly, most of the configuration can be done via API calls, so it's not really necessary to have the gflags configuration in every build.

@jmr Does this make sense?

@jmr jmr self-assigned this Mar 1, 2018
@jmr
Copy link
Member

jmr commented Mar 1, 2018

I'll make gflags and glog optional within the next week or so. The replacement will have a non-empty subset of the functionality.

@acmorrow
Copy link

acmorrow commented Mar 3, 2018

FWIW, finding a replacement for the OpenSSL bignum dependency would be very desirable too, in the long run.

@jmr
Copy link
Member

jmr commented Mar 5, 2018

@acmorrow Can you suggest a good replacement?

@acmorrow
Copy link

acmorrow commented Mar 6, 2018

@jmr - No, not really. At least, not yet. We use the old S2 library in mongodb and at some point we are definitely going to want to upgrade to this new one. At that point, the OpenSSL dependency may be come problematic for us, and we will be happy to help investigate alternatives then.

@jmr
Copy link
Member

jmr commented Mar 6, 2018

gflags and glog are now optional. I'll close this even though OpenSSL is still used, since the original report didn't mention that.

@bdon
Copy link
Contributor

bdon commented Jun 8, 2020

Revisiting this in 2020 - is there any new alternatives to OpenSSL BigNum? Replacing it would go a long way in making static builds possible.

I don't know enough about numerical computing to evaluate whether something like https://github.com/kokke/tiny-bignum-c is viable.

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

No branches or pull requests

6 participants