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

ci: use upstream BUILD file in nlohmann json #14310

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

mering
Copy link
Contributor

@mering mering commented Jun 7, 2024

This reduces maintenance and unblocks people from using the nlohmann_json dependency from BCR when using Bzlmod.


This change is Reviewable

@mering
Copy link
Contributor Author

mering commented Jun 7, 2024

@scotthart @dbolduc could you please trigger the tests? Thanks!

@dbolduc
Copy link
Member

dbolduc commented Jun 7, 2024

/gcbrun

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.22%. Comparing base (4d30f62) to head (d812878).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14310   +/-   ##
=======================================
  Coverage   93.22%   93.22%           
=======================================
  Files        2181     2181           
  Lines      191080   191080           
=======================================
+ Hits       178125   178128    +3     
+ Misses      12955    12952    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alevenberg alevenberg changed the title Use upstream BUILD file in nlohmann json ci: use upstream BUILD file in nlohmann json Jun 7, 2024
Copy link

conventional-commit-lint-gcf bot commented Jun 7, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@scotthart
Copy link
Member

I'm concerned that the upstream BUILD.bazel file specifies alwayslink = True. Our BUILD file does not specify this and does not need it. Adding this flag will only serve to unnecessarily increase the resulting size of the eventual binaries.

@coryan
Copy link
Contributor

coryan commented Jun 11, 2024

I'm concerned that the upstream BUILD.bazel file specifies alwayslink = True. Our BUILD file does not specify this and does not need it. Adding this flag will only serve to unnecessarily increase the resulting size of the eventual binaries.

How? AFAICT, the library is header-only, it does not contain any .o files to increase the size of any binaries. The alwayslink = True seems misguided as it makes no difference, but it also does not seem to harm anything.

Furthermore, if it library had any .o files: we link the library only where it is needed (or we at least we should). I would think nothing can increase "unnecessarily" if the library is already being linked?

@coryan
Copy link
Contributor

coryan commented Jun 11, 2024

Seems like this PR needs a rebase

@mering
Copy link
Contributor Author

mering commented Jun 11, 2024

Seems like this PR needs a rebase

Rebased.

@coryan
Copy link
Contributor

coryan commented Jun 11, 2024

/gcbrun

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

My understanding is that alwayslink functions similarly to --whole-archive, perhaps I'm mistaken. But, as you correctly point out, there are no .o files in this header only library (which probably won't deviate from being header only in the future).

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mering)

@scotthart
Copy link
Member

The conventionalcommits failure should resolve if you edit the commit message to follow the same pattern as the PR title.

@coryan coryan merged commit f8e9863 into googleapis:main Jun 11, 2024
66 of 67 checks passed
@coryan
Copy link
Contributor

coryan commented Jun 11, 2024

The conventionalcommits failure should resolve if you edit the commit message to follow the same pattern as the PR title.

correct, but we can just edit the squash commit message for the same effect (which I just did).

@mering mering deleted the nlohmann-json-build branch June 11, 2024 17:17
@mering
Copy link
Contributor Author

mering commented Jun 11, 2024

Thanks to both of you for your help!

@coryan
Copy link
Contributor

coryan commented Jun 11, 2024

On the contrary, thanks for the PR. We completely missed that this dependency had introduced support for Bazel.

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