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

feat: nlohmann_json add single_include package #2865

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

lromor
Copy link
Contributor

@lromor lromor commented Sep 30, 2024

nlohmann_json provides a new single-header target. Add it as optional target to the the bazel module.

@bazel-io
Copy link
Member

Hello @keith, modules you maintain (nlohmann_json) have been updated in this PR. Please review the changes.

@lromor lromor changed the title feat: nlohmann_json add single-header as default target feat: nlohmann_json add single-header as new target Oct 1, 2024
@lromor lromor force-pushed the add-nlohmann-json-multi-header branch 6 times, most recently from 25ff0a3 to 106bfb5 Compare October 1, 2024 08:33
@lromor lromor changed the title feat: nlohmann_json add single-header as new target feat: nlohmann_json add single_include package Oct 1, 2024
@hzeller
Copy link

hzeller commented Oct 1, 2024

I'd put it in the toplevel BUILD file. Keep the original :json and also provide a :singleheader/json

Reasoning: for projects that also support the 'old' WORKSPACE file, they have a simple way to provide a toplevel BUILD file, but not a nested structure. A project that needs to compile with before and after bzlmod should be able to do that easily.

Example of such a project and how it will be working nicely with a toplevel target : chipsalliance/verible#2268

@lromor lromor force-pushed the add-nlohmann-json-multi-header branch 2 times, most recently from 7b71bd4 to a26590b Compare October 1, 2024 16:53
@lromor lromor marked this pull request as ready for review October 1, 2024 16:58
@hzeller
Copy link

hzeller commented Oct 2, 2024

LGTM

@hzeller
Copy link

hzeller commented Oct 3, 2024

I guess @keith needs to add a review ?

@hzeller
Copy link

hzeller commented Oct 8, 2024

Anything needed to move this PR forward, @keith ?
If you'd like a different name of the target, please suggest it to @lromor .
(as I said, my strong preference is actually singleheader/json, but if you like it to be singleheader-json, so be it (but please no underscore as that would be ugly).

@keith
Copy link
Member

keith commented Oct 8, 2024

i think we should avoid / in the target name since that's too overloaded. note that also affects how targets are created

@lromor lromor force-pushed the add-nlohmann-json-multi-header branch from a26590b to 55de76b Compare October 9, 2024 17:29
@lromor lromor requested a review from keith October 9, 2024 17:29
keith
keith previously approved these changes Oct 9, 2024
keith
keith previously approved these changes Oct 9, 2024
@bazel-io bazel-io dismissed stale reviews from keith and keith October 9, 2024 17:32

Require module maintainers' approval for newly pushed changes.

@keith keith enabled auto-merge (squash) October 9, 2024 17:35
@keith keith merged commit 95eb4fc into bazelbuild:main Oct 9, 2024
19 checks passed
hzeller added a commit to hzeller/verible that referenced this pull request Oct 11, 2024
We used to patch the module to allow the single header version of
the library to be available. Not needed anymore thanks to
@lromor who added it in BCR in
bazelbuild/bazel-central-registry#2865

Now, use the same name for the old-WORKSPACE based build and the
new MODULE based build.
hzeller added a commit to hzeller/verible that referenced this pull request Oct 11, 2024
We used to patch the module to allow the single header version of
the library to be available. Not needed anymore thanks to
@lromor who added it in BCR in
bazelbuild/bazel-central-registry#2865

Now, use the same name for the old-WORKSPACE based build and the
new MODULE based build.

While at it: fix formatting in MODULE.bazel.
hzeller added a commit to hzeller/verible that referenced this pull request Oct 11, 2024
We used to patch the module to allow the single header version of
the library to be available. Not needed anymore thanks to
@lromor who added it in BCR in
bazelbuild/bazel-central-registry#2865

Now, use the same name for the old-WORKSPACE based build and the
new MODULE based build; for that the name we provide in
bazel/jsonhpp.BUILD, and all uses in the repo, had to be changed.

While at it: fix formatting in MODULE.bazel.
hzeller added a commit to hzeller/verible that referenced this pull request Oct 11, 2024
We used to patch the module to allow the single header version of
the library to be available. Not needed anymore thanks to
@lromor who added it in BCR in
bazelbuild/bazel-central-registry#2865

Now, use the same name for the old-WORKSPACE based build and the
new MODULE based build; for that the name we provide in
bazel/jsonhpp.BUILD, and all uses in the repo, had to be changed.

While at it: fix formatting in MODULE.bazel.
hzeller added a commit to hzeller/verible that referenced this pull request Oct 11, 2024
We used to patch the module to allow the single header version of
the library to be available. Not needed anymore thanks to
@lromor who added it in BCR in
bazelbuild/bazel-central-registry#2865

Now, use the same name for the old-WORKSPACE based build and the
new MODULE based build; for that the name we provide in
bazel/jsonhpp.BUILD, and all uses in the repo, had to be changed.

While at it: fix formatting in MODULE.bazel.
asa pushed a commit to asa/bazel-central-registry that referenced this pull request Oct 27, 2024
nlohmann_json provides a new single-header target. Add it as optional
target to the the bazel module.

---------

Co-authored-by: Keith Smiley <[email protected]>
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.

4 participants