nlohmann is not a dependency

[Pamplemousse]

• build with meson is broken (on NixOS) since nlohmann_json: Update to 3.9.1, fix use of internal copy NixOS/nix#5016
• not linked to the binary anywhere
• not listed in the doc/manual/src/installation/prerequisites-source.md

[p01arst0rm]

is nlohmann still included in the build upstream?

[p01arst0rm]

the header is still being referrenced in source upstream, i'd strongly recommend committing there first before changing dependency structures on the fork

[p01arst0rm]

this will cause problems with file inclusion, you cannot chain directories like this in meson, and then append extra directories to the include_directories target; thats the reason its referenced as an array. you should also not use relative paths, its strongly discouraged when creating buildsystem files. please reference the other meson files and model your commits based on them.

[Pamplemousse]

this will cause problems with file inclusion, you cannot chain directories like this in meson, and then append extra directories to the include_directories target;

I don't see the problem it causes...
This is not chaining directories, this is passing several arguments to include_directories.

And BTW, in the documentation, it is said that the include_directories parameters used in library() MUST be an include_directories object (not an array!): https://mesonbuild.com/Reference-manual.html#executable (note that https://mesonbuild.com/Reference-manual.html#library 's documentation states "The keyword arguments for this are the same as for executable with the following additions").

you should also not use relative paths, its strongly discouraged when creating buildsystem files

This is what I get with absolute paths:

src/libutil/meson.build:4:0: ERROR: Tried to form an absolute path to a source dir. You should not do that but use relative paths instead.

please reference the other meson files and model your commits based on them

What do you mean by that?

[p01arst0rm]

once you create an include_directories() target, you cannot append to it. look through the codebase, youll see directories are appended to it per sub directory, instead of at a top level. they are appended to an array which is then passed to the build object; instead of hardcoding all the directories at the top file for that target. using ../ in any capacity causes massive headaches in any buildsystem, and ive lost many hours of work fixing relative paths in MSVC and CMake buildsystems. there is no need to use them, especially in the nix-meson buildsystem, as the .. directory already has a include_directories() target. just reference the already existing target, theres no need to do any kind of path traversal.

[p01arst0rm]

this also breaks header inclusion for build targets. instead of properly inheriting the headers from the build target, it fetches them from the top level src directory. this can result in installing the same headers multiple times in different locations as instead of sharing the same headers, each library installs a set of the same headers of its own.

[p01arst0rm]

same problem here.

[p01arst0rm]

.

[Ericson2314]

If we could do a declare_dependency for the vendored dep I suppose that would abstract over the sin of vendoring, hehe.

[p01arst0rm]

If we could do a declare_dependency for the vendored dep I suppose that would abstract over the sin of vendoring, hehe.

this is more of a regression to how the buildsystem was in 2019, using only the built-in version of nlohmann. nlohmann is a header only dependency Which is not exactly uncommon. nlohmann provides us with pkgconf files for a reason, and we should take advantage of them. the autotools buildsystem has the option to use the system or builtin version of nlohmann, so we must mirror this functionality.

if anything, the header's should be moved to the include directory and referenced from there. best practice, the src directory shouldn't contain and third party code files; but this change would need to be made upstream at a later date, as upstream does not currently use an include directory to configure headers.

[Ericson2314]

@p01arst0rm I think you misunderstand me. Everything that uses nlohmann uses dependency(...) so that the external one would also suffice. There is an option whether or not to do the declare_dependency(..) pathway with the vendored one which is only whether nlohman should be provided internally or externally. Importantly, the use-sites don't have to change regardless of what that option is.

the autotools buildsystem has the option to use the system or builtin version of nlohmann, so we must mirror this functionality.

In essence, I am proposing exactly how we should restore that functionality.

[p01arst0rm]

@p01arst0rm I think you misunderstand me. Everything that uses nlohmann uses dependency(...) so that the external one would also suffice. There is an option whether or not to do the declare_dependency(..) pathway with the vendored one which is only whether nlohman should be provided internally or externally. Importantly, the use-sites don't have to change regardless of what that option is.

the autotools buildsystem has the option to use the system or builtin version of nlohmann, so we must mirror this functionality.

In essence, I am proposing exactly how we should restore that functionality.

ahh, so check pkgconf with dependency() and then fallback to declare_dependency() to create a dependency object? similar to how the libmain libstore etc is handled. That would be a more "correct" way of doing it according to the meson spec :)

[eli-schwartz]

Or just use the nlohmann_json wrap which acts as a dependency() provider and abstracts away everything for you. And you don't even need to check the header into git, just the wrap file.