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.

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

extern from/to_json result in linker error #3657

Closed
2 tasks
steffenb7333 opened this issue Aug 3, 2022 · 12 comments
Closed
2 tasks

extern from/to_json result in linker error #3657

steffenb7333 opened this issue Aug 3, 2022 · 12 comments

Comments

@steffenb7333
Copy link

Description

I have a library, which declares from/to_json as external in its public header as to hide implementation details like this:

#if __has_include(<nlohmann/json_fwd.hpp>)
	#include <nlohmann/json_fwd.hpp>
#else
	#include <nlohmann/json.hpp>
#endif
#include "someclass.hpp"
namespace Library
{
  extern void from_json(const nlohmann::json&, SomeClass&);
  extern void to_json(nlohmann::json&, const SomeClass&);
}

This is compiled with some version of nlohmann-json as there is seldom a need to update/recompile.

Anyhow, that library is then used by a heavily and actively developed codebase which now switched to nlohmann-json 3.11.1. The result is that the linker cannot find the conversion functions b/c the type nlohmann::json is now nlohmann::json_v3_11_1_diag::basic_json for the codebase. And to make matters worse, if I disable diagnostics, it changes again, b/c that is also encoded the namespace name.

Either I have mis-used this library for ages now (without getting any kind of bug/problem) or "abi_macros.hpp" needs some additional thoughts.

Reproduction steps

Declare external conversion functions.
Use different version of this library.

Expected vs. actual results

Should link but does not.

Minimal code example

No response

Error messages

No response

Compiler and operating system

Linux/gcc-12.1.1

Library version

3.11.1

Validation

@falbrechtskirchinger
Copy link
Contributor

Mixing versions is indeed a misuse. v3.11.0 makes that more explicit.

Mixing JSON_DIAGNOSTICS on/off is even worse (and usually results in a segfault anyway).

I'm dealing with some other regressions now and will give this more thought later.

@steffenb7333
Copy link
Author

If version mixing was always a misuse how can a library include any public API using nlohmann::json without being compiled for all existing nlohmann-json-versions when exposing what is now in a separate compilation unit is not an option?

@falbrechtskirchinger
Copy link
Contributor

Mixing different versions in one codebase was always dubious. v3.11.0 made it possible to use different versions as long as they don't interact. Meaning, that you can't pass a basic_json instance created by a function compiled with version A to a function compiled with version B. ("Version" in this context includes different settings, i.e., JSON_DIAGNOSTICS.)

There were never any stability guarantees, but this change does pave the way to make some in the future (although, that point is contentious).

Maybe we should add an option to disable the inline namespace with a big "voids your warranty" warning?

@steffenb7333
Copy link
Author

I would happily void the warranty. And if it is really not intended to mix versions, it should be put into the documentation with a warning label.

As a sidenote: I absolutely support the statement made in #3473 regarding the stable ABI between same major-versions. TBH at least for me that is kind of what is expected except when ABI-breakage is explicitly documented. (And I know that it is far from trivial)

@steffenb7333
Copy link
Author

Addendum: nlohmann_jsonConfigVersion.cmake.in may need patching then, b/c is advertises aforementioned same-major-version compatibility.

@falbrechtskirchinger
Copy link
Contributor

Addendum: nlohmann_jsonConfigVersion.cmake.in may need patching then, b/c is advertises aforementioned same-major-version compatibility.

Good point.

@steffenb7333
Copy link
Author

steffenb7333 commented Aug 3, 2022

Just to be clear, something like this would be the intended way for a public API?

#include <string>
#include <nlohmann/json.hpp> // cannot use json_fwd
#include "someclass.hpp"
namespace Library
{
  namespace detail
  {
    extern void from_json(const std::string&, SomeClass&);
    extern std::string to_json(const SomeClass&);
  }
  void from_json(const nlohmann::json& a_json, SomeClass& a_class)
  {
    detail::from_json(a_json.dump(), a_class);
  }
  void to_json(nlohmann::json& a_json, const SomeClass& a_class)
  {
    a_json = nlohmann::json::parse(detail::to_json(a_class));
  }
}

@nlohmann
Copy link
Owner

nlohmann commented Aug 3, 2022

No, you should not call to_json explicitly. You should define a to_json function in your type's namespace. Inside it, you should trust that conversions just work.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Aug 3, 2022

That's not quite what he's doing. detail::to_json and detail::from_json aren't the ones in namespace nlohmann. They are just used to marshall the data between different library versions.

Edit: Yeah, I think that should work. Not pretty but the only alternative I can think of is rather involved.

@steffenb7333
Copy link
Author

@falbrechtskirchinger That is correct. And I really do not like using std::string as an intermediate representation just to make nlohmann::json work as a type representing "a json document" in the general sense and converting back and forth :-/

Nevertheless the error seems to be on my part, so the issue is resolved.

@falbrechtskirchinger
Copy link
Contributor

FYI, it just occurred to me that it should already be possible to restore the old behavior (untested):

#define NLOHMANN_JSON_NAMESPACE nlohmann
#define NLOHMANN_JSON_NAMESPACE_BEGIN namespace nlohmann {
#define NLOHMANN_JSON_NAMESPACE_END }
#include <nlohmann/json.hpp>

The same should work for the amalgamated json_fwd.hpp.

Whether it is wise to do so depends on the used versions.

@steffenb7333
Copy link
Author

Thanks. It works.

If someone finds this and is interested on how to integrate that project wide in cmake:

find_package(nlohmann_json)
set_target_properties(nlohmann_json::nlohmann_json
PROPERTIES
INTERFACE_COMPILE_DEFINITIONS "NLOHMANN_JSON_NAMESPACE=nlohmann;NLOHMANN_JSON_NAMESPACE_BEGIN=namespace nlohmann {;NLOHMANN_JSON_NAMESPACE_END=}"
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants