Skip to content

Conversation

@verios-google
Copy link
Contributor

Update bazel build to c++17

c++17 has been out for awhile so this is a stable update.

c++17 is a prerequisite for migrating from proto3 to proto editions so would be nice to clear that prerequisite early

Signed-off-by: verios-google <[email protected]>
@verios-google verios-google marked this pull request as draft April 2, 2025 18:27
@verios-google verios-google marked this pull request as ready for review April 2, 2025 18:28
@verios-google
Copy link
Contributor Author

@smolkaj, @chrispsommers, @jafingerhut can I get a review please

@jafingerhut
Copy link
Contributor

@verios-google The changes look pretty small and straightforward, but what I know about Bazel wouldn't fill a teaspoon, so I feel a bit wary about merging this based on my knowledge. If Steffen doesn't get a chance to review it soon, feel free to ping me again in a couple of days and I can approve it.

@verios-google
Copy link
Contributor Author

Thanks, will do if that's the case

@jafingerhut jafingerhut requested a review from smolkaj April 4, 2025 16:13
Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

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

LGTM. p4c is already on C++17 (see eg https://github.com/p4lang/p4c/blob/main/.bazelrc#L6-L8)

@smolkaj
Copy link
Member

smolkaj commented Apr 4, 2025

@chrispsommers could you also PTAL and hit merge if there are no concerns?

@smolkaj smolkaj requested a review from chrispsommers April 4, 2025 18:56
Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

Relying on Steffen, but looks good.

@chrispsommers chrispsommers merged commit 70b5cb0 into p4lang:main Apr 4, 2025
8 checks passed
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