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

Mangle third party libraries #343

Open
mathstuf opened this issue Dec 2, 2015 · 20 comments
Open

Mangle third party libraries #343

mathstuf opened this issue Dec 2, 2015 · 20 comments

Comments

@mathstuf
Copy link
Contributor

mathstuf commented Dec 2, 2015

SMTK should not be installing libcJSON.so, but should instead mangle the library and the symbols to have "smtk" as part of it.

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

Other third party libraries probably have a similar problem.

Cc: @vibraphone

@vibraphone
Copy link
Member

cJSON is small and not widely available as a system package. Because of the way we use it (applications dependent on SMTK will need to use it), I would prefer not to make it a static library that does not get installed. Mangling can be done if absolutely required but until we have an actual need for it I would prefer to avoid it.

sparsehash and clpp are header-only.

The other third-party libraries in SMTK (PyYAML, pugixml, moab) I leave to @BobObara and @robertmaynard .

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

Distros which ship SMTK won't use this one anyways; they'd package cJSON separately and then patch SMTK to use that one. The point here is that SMTK should not install a library using the name of another project, not that the library should be static or whatever. As for mangling, it will be necessary as soon as SMTK is loaded into the same address space as another library using a different cJSON.

@vibraphone
Copy link
Member

@mathstuf I understand that this is desirable but perhaps for now we can install the libraries to a subdirectory of ${prefix}/lib (maybe ${prefix}/lib/smtk/${SMTK_VERSION}/?). It would be much easier than mangling and since we aren't presently colliding with anything, it should not cause problems for CMake.

@robertmaynard
Copy link
Contributor

Shouldn't just providing the option to use a system version of cJSON instead of the built in version alleviate this problem as far as Distro packagers are concerned?

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

we can install the libraries to a subdirectory of ${prefix}/lib (maybe ${prefix}/lib/smtk/${SMTK_VERSION}/?).

Wouldn't work. The SONAME needs to differ.

Shouldn't just providing the option to use a system version of cJSON instead of the built in version alleviate this problem as far as Distro packagers are concerned?

Mangling is to help those not using a system copy.

@vibraphone
Copy link
Member

Shouldn't just providing the option to use a system version of cJSON instead of the built in version alleviate this problem as far as Distro packagers are concerned?

Mangling is to help those not using a system copy.

I think it is fair to assume that people not using a system copy don't have a system copy installed (or they would use it). cJSON is not changing rapidly and not widely distributed, so this is a good assumption. It is also why we should install the cJSON library if SMTK is built with it; otherwise, it is not possible to use an installed SMTK.

@vibraphone
Copy link
Member

@robertmaynard I don't know of a single platform that provides a packaged cJSON. If we create our own homebrew keg, Ubuntu PPA, etc. then we can package cJSON separately if needed. Until then, we need to install it.

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

Until then, we need to install it.

I'm saying to not install it as cJSON. Call it smtkcJSON or something like it instead.

@vibraphone
Copy link
Member

Calling it smtkCJSON would be easy. +1. Can mangling can wait until there's trouble?

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

Yeah, I'm less worried about that.

@BobObara
Copy link
Contributor

BobObara commented Dec 2, 2015

Another possibility for JSON is to use a header only version like: https://github.com/kazuho/picojson - we do this for XML (Pugi).

Bob

Robert M. O'Bara, MEng.
Assistant Director of Scientific Computing

Kitware Inc.
28 Corporate Drive
Suite 101
Clifton Park, NY 12065

Phone: (518) 881- 4931

On Dec 2, 2015, at 10:46 AM, Ben Boeckel notifications@github.com wrote:

Yeah, I'm less worried about that.


Reply to this email directly or view it on GitHub #343 (comment).

@BobObara
Copy link
Contributor

BobObara commented Dec 2, 2015

What problem started this discussion?

Bob
Robert M. O'Bara, MEng.
Assistant Director of Scientific Computing

Kitware Inc.
28 Corporate Drive
Suite 101
Clifton Park, NY 12065

Phone: (518) 881- 4931

On Dec 2, 2015, at 9:08 AM, Ben Boeckel notifications@github.com wrote:

SMTK should not be installing libcJSON.so, but should instead mangle the library and the symbols to have "smtk" as part of it.


Reply to this email directly or view it on GitHub #343.

@vibraphone
Copy link
Member

What problem started this discussion?

I believe Ben is fighting superbuild woes and trying to build against either an installed or build-dir SMTK.

@mathstuf
Copy link
Contributor Author

mathstuf commented Dec 2, 2015

It was something I noticed when adding the target include directories to cJSON so that CMB can use SMTK from a build directory (its own).

@BobObara
Copy link
Contributor

BobObara commented Dec 2, 2015

Are there other libs that we have to worry about or is this the only one? If JSON is the only one I would investigate the header only option.

Bob

Robert M. O'Bara, MEng.
Assistant Director of Scientific Computing

Kitware Inc.
28 Corporate Drive
Suite 101
Clifton Park, NY 12065

Phone: (518) 881- 4931

On Dec 2, 2015, at 12:43 PM, Ben Boeckel notifications@github.com wrote:

It was something I noticed when adding the target include directories to cJSON so that CMB can use SMTK from a build directory (its own).


Reply to this email directly or view it on GitHub #343 (comment).

@vibraphone
Copy link
Member

Are there other libs that we have to worry about or is this the only one? If JSON is the only one I would investigate the header only option.

I think switching from cJSON to picojson would be an order of magnitude more work than mangling cJSON, which is an order of magnitude more work than just changing the library name, which I think we have agreed will suffice for now.

@BobObara
Copy link
Contributor

BobObara commented Dec 2, 2015

I have no problem with this approach but I do have to ask is the JSON support confined to a 2 classes like XML is in SMTK?

Bob

Robert M. O'Bara, MEng.
Assistant Director of Scientific Computing

Kitware Inc.
28 Corporate Drive
Suite 101
Clifton Park, NY 12065

Phone: (518) 881- 4931

On Dec 2, 2015, at 1:58 PM, David Thompson notifications@github.com wrote:

Are there other libs that we have to worry about or is this the only one? If JSON is the only one I would investigate the header only option.

I think switching from cJSON to picojson would be an order of magnitude more work than mangling cJSON, which is an order of magnitude more work than just changing the library name, which I think we have agreed will suffice for now.


Reply to this email directly or view it on GitHub #343 (comment).

@robertmaynard
Copy link
Contributor

In short, no. json parsing/conversion is done by numerous classes in smtk.

@vibraphone
Copy link
Member

... is the JSON support confined to a 2 classes like XML is in SMTK?

Close.

  1. There are delegate classes that bridges may provide in order to process additional JSON records not handled by Import/ExportJSON.
  2. The mesh interface uses it directly in the "JSON" backend.
  3. The remus remote bridge and CMB forwarding operators use cJSON directly for non-model-related tasks (as an RPC encoding).

@vibraphone vibraphone reopened this Dec 2, 2015
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

4 participants