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

build: install .pc and .cmake files to share/ #3619

Merged
merged 1 commit into from
Jul 29, 2022
Merged

build: install .pc and .cmake files to share/ #3619

merged 1 commit into from
Jul 29, 2022

Conversation

Tachi107
Copy link
Contributor

@Tachi107 Tachi107 commented Jul 28, 2022 •
edited
Loading

As nlohmann_json is a header-only library, its pkg-config and cmake config files should be installed to share/ (GNUInstallDirs' DATADIR), as share/ is the canonical directory where architecture-independent files should be stored, while lib/ is for architecture-dependent stuff (see the FHS).

Apart from being technically correct, installing to share/ can help with cross-compiling, for example in Debian-based environments. If you're interested, I'd suggest reading this thread.

Related to #1697

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

@Tachi107
build: install .pc and .cmake files to share/
886ee69 
As nlohmann_json is a header-only library, its pkg-config and cmake
config files should be installed to `share/` (GNUInstallDirs' DATADIR),
as `share/` is the canonical directory where architecture-independent
files should be stored, while `lib/` is for architecture-dependent stuff
(see the [FHS][]).

Apart from being technically correct, installing to `share/` can help
with cross-compiling, for example in Debian-based environments. If
you're interested, I'd suggest reading this [thread][].

Related to nlohmann#1697

[FHS]: https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.html#usrshareArchitectureindependentData
[thread]: marzer/tomlplusplus#165 (comment)
@Tachi107 Tachi107 requested a review from nlohmann as a code owner July 28, 2022 14:28
@Tachi107
Copy link
Contributor Author

@uhoreg, as you're the maintainer of the nlohmann-json3 Debian package, would you like to share some comment?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 886ee69 on Tachi107:patch-1 into 2d48a4d on nlohmann:develop.

@uhoreg
Copy link
Contributor

uhoreg commented Jul 29, 2022

@uhoreg, as you're the maintainer of the nlohmann-json3 Debian package, would you like to share some comment?

I don't know much about cross-compiling, so I can't say much about how it would affect that, but it does at least seem plausible that putting it in share would work better for cross-compiling.

In general, share is for architecture-independent files, and lib is for architecture-dependent files. Since this library is header-only, share seems like the logical place for those files. (Even with libraries that are architecture-dependent, some of them place their .pc files in share, since the contents of the .pc file are the same on every architecture.)

falbrechtskirchinger
falbrechtskirchinger approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Looks good to me.

nlohmann
nlohmann approved these changes Jul 29, 2022
View reviewed changes
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Jul 29, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jul 29, 2022
@nlohmann nlohmann merged commit 6576c3f into nlohmann:develop Jul 29, 2022
@Tachi107
Copy link
Contributor Author

Thanks for merging :)

@Tachi107 Tachi107 deleted the patch-1 branch July 29, 2022 08:35
@nlohmann
Copy link
Owner

Thanks for opening the PR!

bob-beck pushed a commit to openbsd/ports that referenced this pull request Feb 4, 2023
@landryb
textproc/nlohmann-json: update to 3.11.2
54dd6c8 
- cmake/.pc files are now installed to share/ as arch-independant, cf
  nlohmann/json#3619 for the rationale
from kn@:
- set NO_BUILD as this is a header-only port
- Defer building tests to do-test target
- Drop NO_TEST, all tests succeed (except the 4 that try to reach network via
  git clone)

all consumers (mtxclient, nheko and mkvtoolnix) build fine with it.

should fix a warzone2100 build failure seen by tb@
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlohmann nlohmann nlohmann approved these changes

@falbrechtskirchinger falbrechtskirchinger falbrechtskirchinger approved these changes

Assignees

@nlohmann nlohmann

Labels
release item: 🔨 further change
Projects
None yet
Milestone
Release 3.11.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Tachi107 @coveralls @uhoreg @nlohmann @falbrechtskirchinger