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.

Sign up for GitHub

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

Jump to bottom

Fixes related to external-json-parser #165

Merged
merged 3 commits into from
Mar 4, 2023
Merged

Fixes related to external-json-parser #165

merged 3 commits into from
Mar 4, 2023

Conversation

arkamar
Copy link
Contributor

@arkamar arkamar commented Dec 4, 2022

This PR fixes multiple issues for those who wish to use system libjsonparser. First two commits are related to external plugins, the last commit corrects which json.h should be used when external-json-parser is set. See commit messages for more info.

jelmer
jelmer reviewed Dec 4, 2022
View reviewed changes
@arkamar
Copy link
Contributor Author

arkamar commented Jan 24, 2023

Would it be possible to merge this? It would be helpful for Gentoo package.

@jelmer jelmer mentioned this pull request Feb 2, 2023
Merged
jelmer
jelmer requested changes Feb 28, 2023
View reviewed changes
Copy link
Member

@jelmer jelmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take a look at the failing tests?

@arkamar
configure: append conditionally json-parser to pc file
958d505 
json-parser needs to be appended to Requires section of pkg-config file
when bitlbee is configured to use external-json-parser. This change is
needed for external plugins like bitlbee-steam.
@arkamar
Makefile: do not install json.h when system json-parser is used
b34d3c2 
install-dev target should not install bundled json.h when bitlbee is
configured to use external-json-parser.
@arkamar
Use correct json.h header file with respect to external_json_parser v…
ca74f4d 
…alue

The preprocessor must include correct json.h header file with respect to
external_json_parser value, otherwise function prototypes and other
definitions do not need to correspond with object used for linking.

The state before this commit is that local version lib/json.h is used
always for compilation and external_json_parser variable controls if
local lib/json.o or global libjsonparser.so will be linked.

In order to fix this problem, #include directives in lib/json_util.h and
lib/oauth2.c were changed from "json.h" to <json.h> and preprocessor -I
flags were moved after conditional json-parser flags, which is enough
for solving the issue. Additionally, USE_EXTERNAL_JSON_PARSER macro is
exported when external-json-parser is used and it is used in lib/json.h
to trigger an error message, which should prevent similar mistakes in
future.
@arkamar
Copy link
Contributor Author

arkamar commented Feb 28, 2023

Merge conflict was incorrectly resolved. I have rebased the branch on top of master.

@arkamar arkamar requested a review from jelmer March 4, 2023 15:25
@jelmer jelmer merged commit 82149f4 into bitlbee:master Mar 4, 2023
@arkamar arkamar deleted the system-json-parser branch March 5, 2023 08:01
@arkamar
Copy link
Contributor Author

arkamar commented Mar 5, 2023

Thanks!

@msva
Copy link

msva commented Apr 5, 2023

Unfortunatelly, those changes leads to build failures here, as json.h somewhy gets included from ./lib/ instead of from /usr/include/json-parser/ 🤷‍♂️
build.log

@arkamar
Copy link
Contributor Author

arkamar commented Apr 5, 2023

changes from this PR are ok, I tested them in live ebuild. The failure is most probably caused by improper revert in commit 59c9fa4. The problem is that configure must append CFLAGS lines to Makefile.settings in correct order because order of -I flags is important for the compiler.

Which does not happen right now in commit 92a03a0

x86_64-gentoo-linux-musl-gcc -c -march=native -O2 -pipe -frecord-gcc-switches -flto -Werror=odr -Werror=lto-type-mismatch -Werror=strict-aliasing -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999 -I/var/tmp/portage/net-im/bitlbee-99
99/work/bitlbee-9999/lib -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/protocols -I. -DHAVE_CONFIG_H -D_GNU_SOURCE -Wall -Wformat -Werror=format-security -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -pthread  -I/usr/inclu
de/json-parser  -DUSE_EXTERNAL_JSON_PARSER -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999 -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/lib -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/protocols -I. -MMD -M
F .depend/json_util.o.d -fPIE json_util.c -o json_util.o
In file included from json_util.h:24,
                 from json_util.c:28:
/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/lib/json.h:32:2: error: #error Bitlbee was configured to use system json-parser, this header file should not be used.
   32 | #error Bitlbee was configured to use system json-parser, this header file should not be used.
      |  ^~~~~
make[1]: *** [Makefile:48: json_util.o] Error 1
make[1]: Leaving directory '/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/lib'

See duplicated -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999 -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/lib -I/var/tmp/portage/net-im/bitlbee-9999/work/bitlbee-9999/protocols -I., first appearance should not be there.

I added the #error there to actually catch these situations. It is important to use json.h from correct place, otherwise it could result in binary incompatibility, when structures are different, for example...

@robert-scheck
Copy link
Contributor

I'm not sure how this shall work: json.h is included by BitlBee code, however the system json-parser package usually provides json.h in /usr/include/json-parser/json.h (at least on Debian and Fedora). Thus this would IMHO require in BitlBee code <json-parser/json.h>; just adding -I/usr/include/json-parser (like Gentoo does?) seems to be wrong, because there might be also other json.h on default include paths.

@jelmer
Copy link
Member

jelmer commented Apr 5, 2023

the system json-parser is meant to be included as <json.h>, its pkg-config adds -I/usr/include/json-parser

The best way to ensure it gets included before the bitlbee json.h is probably to add its cflags to the front of the cflags.

@robert-scheck
Copy link
Contributor

pkg-config adds -I/usr/include/json-parser

Thank you for the pointer!

@robert-scheck robert-scheck mentioned this pull request Apr 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jelmer jelmer jelmer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arkamar @msva @robert-scheck @jelmer