Build system fixes #47
Conversation
Running -Werror during development is fine, but not in released code. See [1] for further details. [1] https://blog.flameeyes.eu/2009/02/future-proof-your-code-dont-use-werror
|
While I think removing the private files from json.h is definitely the right thing to do, I am not sure I can merge immediately. I have not yet checked if currently rsyslog and helpers depend on some of these definitions. If they do, let's see if I manage to get all changed until the next release. If not, we need to postpone that part of the merge as well, because I would like to do as much cleanup as possible with that release. |
| @@ -25,7 +25,6 @@ libfastjsoninclude_HEADERS = \ | |||
|
|
|||
| libfastjson_la_LDFLAGS = -version-info 3:0:0 -no-undefined @JSON_BSYMBOLIC_LDFLAGS@ \ | |||
| -export-symbols-regex '^fjson_.*' | |||
| libfastjson_la_CPPFLAGS = -Werror | |||
There was a problem hiding this comment.
What would you consider to be an alternative when release building? Using -Werror in CFLAGS does not play well with configure (there are many post on that problem on the Internet). So would you recommend to do - a configure option?
There was a problem hiding this comment.
What exactly is the problem with passing -Werror via CFLAGS? Do you have any references?
That said, a good middle ground could be something like [1].
During development you could build with
--enable-more-warnings=error.
The important part is, that hard-coding -Werror is bad and should be removed.
[1] https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/m4/compiler_warnings.m4
There was a problem hiding this comment.
2016-04-07 16:09 GMT+02:00 Michael Biebl [email protected]:
In Makefile.am
#47 (comment):@@ -25,7 +25,6 @@ libfastjsoninclude_HEADERS = \
libfastjson_la_LDFLAGS = -version-info 3:0:0 -no-undefined @JSON_BSYMBOLIC_LDFLAGS@
-export-symbols-regex '^fjson_.*'
-libfastjson_la_CPPFLAGS = -WerrorWhat exactly is the problem with passing -Werror via CFLAGS? Do you have
any references?
The problem is that -Werror make autoconf AC_CHECK_FUNC checks think
features are not available, just because the way configure works. Check
here for a sample using strcmp:
https://gist.github.com/rgerhards/723c3777158fb882e15c9179c1fa76c8#file-config-log-L415
That said, I noticed that in the mean time I was able to remove all
AC_CHECK_FUNC, so it is not an issue at the moment.
That said, a good middle ground could be something like [1].
During development you could build with
--enable-more-warnings=error.
That's probably a better method anyhow.
The important part is, that hard-coding -Werror is bad and should be
removed.[1]
https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/m4/compiler_warnings.m4
read it and agree, do not question the need, but think about how to do
it. I need to say that I just recently added -Werror after I banged my head
for some hours in not getting travis up due to above-mentioned problem.
Rainer
—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/rsyslog/libfastjson/pull/47/files/14be0062e7fe2f79d0428596c15b3e609d8d195a#r58877067
There was a problem hiding this comment.
Ok, let's remove it and I'll promise to prepare a follow-up fix which steals the NM macro and includes that.
There was a problem hiding this comment.
Nice, I see that this has been included in autoconf-archive [1].
Actually, I don't know if the NM is based on that or inspired that macro.
I guess we should prefer the one from autoconf-archive.
[1] https://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html
There was a problem hiding this comment.
ie., let's just drop the usage of Werror until we re-introduce it via AX_COMPILER_FLAGS
There was a problem hiding this comment.
2016-04-07 16:55 GMT+02:00 Michael Biebl [email protected]:
In Makefile.am
#47 (comment):@@ -25,7 +25,6 @@ libfastjsoninclude_HEADERS = \
libfastjson_la_LDFLAGS = -version-info 3:0:0 -no-undefined @JSON_BSYMBOLIC_LDFLAGS@
-export-symbols-regex '^fjson_.*'
-libfastjson_la_CPPFLAGS = -WerrorWhy does removing Werror from libfastjson_la_CPPFLAGS break AC_CHECK_FUNCS?
I can understand that adding -Werror via CFLAGS can have an influence on
AC_CHECK_FUNCS, but dropping it should be safe.At this point, my main concern is travis and not to re-open old problems. I
have now managed to get travis require warning-free builds, I would like to
keep this while we improve the system.
There was a problem hiding this comment.
2016-04-07 16:56 GMT+02:00 Michael Biebl [email protected]:
In Makefile.am
#47 (comment):@@ -25,7 +25,6 @@ libfastjsoninclude_HEADERS = \
libfastjson_la_LDFLAGS = -version-info 3:0:0 -no-undefined @JSON_BSYMBOLIC_LDFLAGS@
-export-symbols-regex '^fjson_.*'
-libfastjson_la_CPPFLAGS = -Werrorie., let's just drop the usage of Werror until we re-introduce it via
AX_COMPILER_FLAGSOK, that works. But I would like to check on the internal headers before
merging. It's easy if I complete my current task, but mixing this merge and
the tasks costs me much more coordination over the projects. So bear with
me for a couple of hours. (As I wrote in the overall issue, that commit
breaks rsyslog build).
There was a problem hiding this comment.
2016-04-07 16:59 GMT+02:00 Rainer Gerhards [email protected]:
2016-04-07 16:56 GMT+02:00 Michael Biebl [email protected]:
In Makefile.am:
@@ -25,7 +25,6 @@ libfastjsoninclude_HEADERS = \
libfastjson_la_LDFLAGS = -version-info 3:0:0 -no-undefined
@JSON_BSYMBOLIC_LDFLAGS@
-export-symbols-regex '^fjson_.*'
-libfastjson_la_CPPFLAGS = -Werrorie., let's just drop the usage of Werror until we re-introduce it via
AX_COMPILER_FLAGSOK, that works. But I would like to check on the internal headers before
merging. It's easy if I complete my current task, but mixing this merge and
the tasks costs me much more coordination over the projects. So bear with me
for a couple of hours. (As I wrote in the overall issue, that commit breaks
rsyslog build).
In other words: I don't think it doesn't really matter if we merge
today, tomorrow or monday, but it eases my life considerably if we do
not merge immediately.
There was a problem hiding this comment.
No worries. Please take the time to review carefully before you merge.
|
I tried in a temporary branch. Without the internal includes, at least some things break. Hopefully all is related to #46 (which uses internal structures). I am working on that one, will retry the merge when I am finished with it. |
|
Ok, so should I close this PR, open a new one without the header file changes for now and we deal with that in a separate PR? |
|
keep it open at the moment. Hopefully I am done with the other work in a day or two. More comments follow. |
This also reduces the visual clutter when updating the version info.
Those are no longer public ABI, so they shouldn't be public API either. Fix json.h to no longer reference those files.
Since json.h after commit 25daae3 no longer includes the private headers, make the test suite include the individual headers as needed. An alternative would be to create a "json_internal.h", which the tests can include and which reference all internal headers.
mbiebl
force-pushed
the
build-system-fixes
branch
from
d6d5a0c to
6fafb53
Compare
|
FYI: I have this basically working on my test branch, but it triggers some problems in rsyslog which I need to look into before finally merging (else I break CI). |
Various smaller fixes / improvements.
The next step would be, moving the private header files from libfastjsoninclude_HEADERS to libfastjson_internal_la_SOURCES, i.e. make sure those are no longer installed.
I've updated this PR to include the changes for the header files.
Please comment on d6d5a0c, especially if you'd prefer a json_internal.h for the tests.