Skip to content

Conversation

@joto
Copy link
Collaborator

@joto joto commented Nov 22, 2024

See individual commits.

joto added 3 commits November 22, 2024 12:10
* Now needs a minimum of C++14, because the Protobuf library used in
  tests uses Absl which needs C++14
* Some changes to CMake config to make writer_tests compile on macOS
  and Windows
* Updated Github action CI workflow
@joto
Copy link
Collaborator Author

joto commented Nov 22, 2024

Last time I looked I was the maintainer of this software. But Mapbox has locked down the repo and I can't merge my own changes any more. Anybody at Mapbox seeing this? Please either give me some rights so that I can merge stuff or approve the PR.

Copy link
Contributor

@springmeyer springmeyer left a comment

Choose a reason for hiding this comment

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

Looks great to me. And I presume that the protozero headers will still continue to work with C++11 if anyone needed (as these changes are only build system + catch related)?

@springmeyer
Copy link
Contributor

springmeyer commented Dec 6, 2024

@flippmoke @brunoabinader - can either of you add a review or are able to merge this?

@kkaefer
Copy link
Member

kkaefer commented Dec 10, 2024

@joto sorry for the delay. Could you please fix the remaining build failure?

@joto
Copy link
Collaborator Author

joto commented Dec 10, 2024

@kkaefer The build failure was not introduced by anything I changed in the code but by me updating the Github action scripts so they run on new machines. So it is a good thing we are catching that error now while it is only showing up on Debian experimental. So this PR is only one step, at least we know better where we stand after this.

I am trying to get this to a point where I can maybe fix that build failure but the issues is with the test framework which hasn't been updated in many years which the new clang version doesn't like for some reason. But to move forward I first need the CI to work.

@joto joto requested a review from kkaefer December 12, 2024 07:46
@kkaefer
Copy link
Member

kkaefer commented Dec 19, 2024

The build failure was not introduced by anything I changed in the code

Maybe, but the fix looks trivial (add -Wno-c++20-extensions)

joto added 2 commits December 19, 2024 10:53
It is unclear what the problem is, but this should "fix" it.
@joto joto force-pushed the update-some-stuff branch from 7d5b3ec to 16e41ee Compare December 19, 2024 10:06
@joto
Copy link
Collaborator Author

joto commented Dec 19, 2024

Usually I don't like to disable warnings, but in this case I guess there is no way around for the moment. Until I can maybe get a newer version of the tests to run that doesn't require this any more. Done now.

@kkaefer
Copy link
Member

kkaefer commented Dec 19, 2024

Well, the original warning was

/__w/protozero/protozero/test/t/bool/writer_test_cases.cpp:8:1: error: passing no argument for the '...' parameter of a variadic macro is a C++20 extension [-Werror,-Wc++20-extensions]
    8 | TEMPLATE_TEST_CASE("write bool field and check with libprotobuf", "",
      | ^

@kkaefer kkaefer merged commit 5cc0ee5 into mapbox:master Dec 19, 2024
28 checks passed
@joto joto deleted the update-some-stuff branch December 19, 2024 13:41
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.

3 participants