Skip to content

Disable building json2cbor.c in tinycbor, and which doesn't build on macOS/BSDs when cjson is installed#464

Merged
kkysen merged 2 commits intomasterfrom
kkysen/tinycbor-disable-cjson
Jun 28, 2022
Merged

Disable building json2cbor.c in tinycbor, and which doesn't build on macOS/BSDs when cjson is installed#464
kkysen merged 2 commits intomasterfrom
kkysen/tinycbor-disable-cjson

Conversation

@kkysen
Copy link
Contributor

@kkysen kkysen commented Jun 27, 2022

Fixes #353.

This unconditionally disables building tools/json2cbor/ in tinycbor, which isn't needed for our uses of tinycbor and is an optional dependency. Normally it is autodetected when cjson is found, and it still is, but now we append to the generated .config file to explicitly disable it.

On macOS/BSD, tools/json2cbor/json2cbor.c doesn't build because it defines _POSIX_C_SOURCE, which in BSD libc, prevents asprintf from being declared, which is used by tools/json2cbor/json2cbor.c. This can be fixed by removing the _POSIX_C_SOURCE define, but that'd have to be fixed in a patch or upstream in https://github.com/intel/tinycbor, which it probably should be, but the fix would take longer.

However, either way, we don't need json2cbor, so we can just always disable it, and it's better to be explicit about our dependencies than unknowingly having an optional dependency on cjson that affects the build.

…in `tinycbor`, and which doesn't build on macOS/BSDs when `cjson` is installed.
@kkysen
Copy link
Contributor Author

kkysen commented Jun 27, 2022

@gcxfd, can you see if this fixes your tinycbor issue when compiling from cargo install c2rust?

@kkysen kkysen marked this pull request as draft June 27, 2022 16:02
@kkysen kkysen marked this pull request as ready for review June 27, 2022 17:05
@kkysen kkysen requested a review from fw-immunant June 27, 2022 17:52
@kkysen
Copy link
Contributor Author

kkysen commented Jun 28, 2022

It's fixed upstream, too, now: intel/tinycbor#221.

So we don't need this fix anymore (if we upgrade our tinycbor version), but I think it's still worth it to have, because otherwise we're pulling in an unneeded dependency, but only sometimes, and there could be some other future error there.

@kkysen
Copy link
Contributor Author

kkysen commented Jun 28, 2022

intel/tinycbor#221 has now been tested on macOS by @boyland-pf and it works, so we just need to test this one now (with cjson installed globally) if anyone with a mac can:

cargo install c2rust --git https://github.com/immunant/c2rust --branch kkysen/tinycbor-disable-cjson

Once that's tested, we should be able to merge this I think.

@boyland-pf
Copy link
Contributor

This also compiles on mac.

Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

Thanks for tracking down the issue here. Fix looks good.

@kkysen kkysen merged commit 63bbfb3 into master Jun 28, 2022
@kkysen kkysen deleted the kkysen/tinycbor-disable-cjson branch July 9, 2022 04:06
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.

tinycbor dependency doesn't build on macOS/BSD when cjson is installed

3 participants