-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixes for cmake 2.8.12 + link issue on AIX 6.1/cc 11.01 #626
Conversation
CMakeLists.txt
Outdated
cmake_policy(SET CMP0048 NEW) | ||
endif() | ||
|
||
set(LIB_MAJOR_VERSION "0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you setting these variables twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, why set these at all, if we're not using them anywhere? It seems like there's something missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.Sorry for that. I will update it again. 2.cmake 2.8 doesn't support VERSION in project, so instead of project(json-c VERSION 0.13.1), LIB_VERSION_STRING needs to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LIB_VERSION_STRING isn't used. You're setting all those variables for no apparent purpose.
However, we do use the PROJECT_VERSION* variables, so I don't quite see how these changes could work. I think you're going to need to set those variables inside the < 3.0 block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved them as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I know for sure is that cmake 2.8.12 doesn't support version with project command, so there can't be anything setting PROJECT_VERSION
. The below is from https://cmake.org/cmake/help/v2.8.12/cmake.html#command:project.
project: Set a name for the entire project.
project(<projectname> [languageName1 languageName2 ... ] )
Sets the name of the project. Additionally this sets the variables <projectName>_BINARY_DIR and <projectName>_SOURCE_DIR to the respective values.
Optionally you can specify which languages your project supports. Example languages are CXX (i.e. C++), C, Fortran, etc. By default C and CXX are enabled. E.g. if you do not have a C++ compiler, you can disable the check for it by explicitely listing the languages you want to support, e.g. C. By using the special language "NONE" all checks for any language can be disabled.
Whereas, cmake 3.0.2 supports version in project command:
project(<PROJECT-NAME> [LANGUAGES] [<language-name>...])
project(<PROJECT-NAME>
[VERSION <major>[.<minor>[.<patch>[.<tweak>]]]]
[LANGUAGES <language-name>...])
So, I've made another change:
1.I have taken out version settings:
if (CMAKE_VERSION VERSION_LESS 3.0)
project(json-c)
else()
project(json-c VERSION 0.13.1)
endif()
2.Put version setting for cmake 2.8 and specify target VERSION properties
set_property(TARGET json-c PROPERTY C_STANDARD 99)
set_property(TARGET json-c-static PROPERTY C_STANDARD 99)
set_target_properties(json-c-static PROPERTIES OUTPUT_NAME json-c)
if (CMAKE_VERSION VERSION_LESS 3.0)
# Fixes for CMake 2.8.12
# Reference: https://github.com/Tencent/rapidjson/issues/1154
set(LIB_MAJOR_VERSION "0")
set(LIB_MINOR_VERSION "13")
set(LIB_PATCH_VERSION "1")
set(LIB_VERSION_STRING "${LIB_MAJOR_VERSION}.${LIB_MINOR_VERSION}.${LIB_PATCH_VERSION}")
set_target_properties(json-c PROPERTIES VERSION ${LIB_VERSION_STRING})
set_target_properties(json-c-static PROPERTIES VERSION ${LIB_VERSION_STRING})
endif ()
Here's the install result and I can see 0.13.1 is used(previously libjson-c.so.0.13.1 was not created):
Install the project...
-- Install configuration: ""
-- Installing: /prj/lucas/scm298514_9.32dev/extern/Linux/local64/lib/libjson-c.so.0.13.1
-- Installing: /prj/lucas/scm298514_9.32dev/extern/Linux/local64/lib/libjson-c.so
-- Installing: /prj/lucas/scm298514_9.32dev/extern/Linux/local64/lib/libjson-c.a
I wish this would make sense to you this time.
P.S. make distcheck
is not available on 0.13.1, so I couldn't run it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't realize this was specific to the 0.13 branch. Is there any reason you can't use 0.14, or even the current code? I would rather not make fixes to a branch that haven't yet been done on master.
Also, even on the 0.13 branch there is a reference to PROJECT_VERSION. Why are you refusing to use PROJECT_ VERSION* variables?! Are you saying that cmake 2.8 will error out if you try to set any of those variables?
From my testing, if you don't set those variables (well, at least PROJECT_VERSION) then the json-c.pc file ends up invalid, with an empty version. (See the set(VERSION ${PROJECT_VERSION})
line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.I have to use same version on Cent OS 6.8 and AIX 6.1. there's are only json-c 0.12, 0.13, and 0.13.1, so 0.14 rpm is NOT available on AIX 6.1 at the moment, so I've tried to build 0.13.1 on Cent OS 6.8 which only has cmake 2.8.12 at the moment.
2.As I wrote, cmake 2.8.12 doesn't seem to support PROJECT_VERSION. I looked at the document of cmake 2.8.12 and there's nothing like PROJECT_VERSION. If you want, I can try later, but I'd like to ask you to have a look at document of cmake 2.8.12. Installing cmake 3.x would be another time consuming job which should be done by my company's IT team, so I wouldn't want to go that way so long as I can use same version of json-c.
3.With my latest change, as you can see, no matter how it worked, make install
installed -- Installing: /prj/lucas/scm298514_9.32dev/extern/Linux/local64/lib/libjson-c.so.0.13.1
, so it should be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
ah, ok. Though if 0.14 or the master branch don't build on AIX, I'd certainly like to know. I adjusted CMakeLists.txt on master, and applied your fix to linkhash.h, so I expect that build to be ok if you care to go down that path.
-
It's irrelevant whether cmake supports (i.e. sets) that variable. json-c's CMakeLists.txt uses it, so it needs to be set one way or another. Please see commit fe308b8 for the fix that I pushed to master.
(incidentally, building cmake and installing locally isn't all that hard, though I can understand wanting to avoid the need to do that) -
I'm not going to merge something that merely goes as far as not completely failing on
make install
when I know that it's generating at least some bad output. I cherry picked the version of the linkhash.h fix that I just pushed to master (1c6086a), but there have been too many changes to simply cherry pick the CMakeLists.txt change. If you respin your PR so it sets the PROJECT_VERSION_* vars similar to fe308b8, I'll be happy to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'd like to, but I won't be able to build 0.14 on AIX 6.1 as I am not allowed to install it on my own.
2 and 3. I will try the commit to see if it works and let you know tomorrow.
… and explicitly defining the PROJECT_VERSION* variables.
…ut invert the test to make it a little easier to understand.
…ut invert the test to make it a little easier to understand. (cherry picked from commit 1c6086a)
…ng the PROJECT_VERSION* variables. (cherry picked from commit fe308b8)
Closing without merging. I pulled over the relevant pieces of fe308b8 to the json-c-0.13 branch, so this PR is no longer relevant. |
Fixes for cmake 2.8.12
Build failed with cmake 2.8.12
Reference: Tencent/rapidjson#1154
With the fixes
Test with hello world application
Test environment
Linking issue on AIX 6.1 with cc V11.1
When linking applicatio to json-c-0.13.1(installed by json-c-0.13.1-1.aix5.1.ppc.rpm from http://www.oss4aix.org/download/RPMS/json-c/), cc complained heaps of things which were fixed by the change.
Test environment