Skip to content

Static/shared #263

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

Merged
merged 9 commits into from
Apr 23, 2015
Merged

Conversation

cdunn2001
Copy link
Contributor

(created from several separate branches by @ya1gaurav)

Let's see if it builds....

Currently JSONCPP_LIB_BUILD_SHARED variable is used as option to build static/shared libraries.
The current patch uses standard CMake variables for this.
Such a workaround is done in open-source-parsers#51
Current patch will make it generic.
Replace JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Replace JSONCPP_LIB_BUILD_STATIC => BUILD_STATIC_LIBS
Removed workaround  open-source-parsers#51
Removed OPTION for shared/static in this file.
Replaced JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Moved definition DJSON_DLL to line 11.
Replaced JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Moved flag JSON_DLL to line no 8.
Replaced JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Replaced JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Replaced JSONCPP_LIB_BUILD_STATIC => BUILD_STATIC_LIBS
Replace JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Replace JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
Replace JSONCPP_LIB_BUILD_SHARED => BUILD_SHARED_LIBS
@cdunn2001
Copy link
Contributor Author

Let's discuss this now. What was broken? What's the goal of this change?

@ya1gaurav
Copy link
Contributor

The goal of these changes is to use standard CMake variable as option for building either shared/static library. Currently it is using JSONCPP_LIB_BUILD_SHARED & JSONCPP_LIB_BUILD_STATIC.
It will avoid extra variables, as there exists standard variable for same purpose.
Also, as I checked these variables are not used for some other purpose.

@cdunn2001
Copy link
Contributor Author

According to a comment elsewhere, using the non-standard variables means that this library does not work well as a subproject. Is that your point?

I tried to learn why the custom variables were chosen. It seems to have been arbitrary, in the original (eafd702) commit for cmake build-config.

I think AppVeyor failed a build only because of a problem contacting GitHub. That's fine.

cdunn2001 added a commit that referenced this pull request Apr 23, 2015
Use standard **cmake** variables, to support superprojects better.

- `JSONCPP_LIB_BUILD_SHARED` -> `BUILD_SHARED_LIBS`
- `JSONCPP_LIB_BUILD_STATIC` -> `BUILD_STATIC_LIBS`
@cdunn2001 cdunn2001 merged commit ae177fd into open-source-parsers:master Apr 23, 2015
@ya1gaurav
Copy link
Contributor

Actually my motivation is one of the patch i saw - #51
As it is included as work around for standard variable, then why using the other variables like JSONCPP_LIB_BUILD_SHARED, it seems duplicate. Isn't it?

@ya1gaurav
Copy link
Contributor

I have one more question regarding flag "JSON_DLL_BUILD".
DLL are for Windows, is this flag generic for shared library or only for windows in Jsoncpp?
If it is for windows only, should be written under "if(WIN32)".

@cdunn2001
Copy link
Contributor Author

should be written under "if(WIN32)"

Yes. But it's mingled with the CPPTL code, which I cannot test. I am hoping not to break that since @blep (the original author) still uses it. It's a minor issue.

Thanks for fixing these variables names.

@cdunn2001 cdunn2001 deleted the static-shared branch April 24, 2015 01:20
@ya1gaurav
Copy link
Contributor

In Readme file, i can see below:
If JsonCpp was built as a dynamic library on Windows, then your project needs to define the macro JSON_DLL.
It think it means for windows only.

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

2 participants