Skip to content

Update BoringSSL to 2023-08-28. #391

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

Merged
merged 5 commits into from
Apr 29, 2024

Conversation

martijneken
Copy link
Contributor

Fixes build warning-as-error:

external/boringssl/src/crypto/x509/t_x509.c:326:18: error: variable 'l' set but not used [-Werror,-Wunused-but-set-variable]
    int ret = 0, l, i;
                 ^

Fixes build warning-as-error:
```
external/boringssl/src/crypto/x509/t_x509.c:326:18: error: variable 'l' set but not used [-Werror,-Wunused-but-set-variable]
    int ret = 0, l, i;
                 ^
```

Signed-off-by: Martijn Stevenson <[email protected]>
Signed-off-by: Martijn Stevenson <[email protected]>
@martijneken
Copy link
Contributor Author

@martijneken martijneken requested a review from mpwarres April 17, 2024 04:38
@martijneken
Copy link
Contributor Author

Friendly ping @PiotrSikora

@PiotrSikora
Copy link
Member

LGTM, but those should be 2 separate PRs.

@martijneken
Copy link
Contributor Author

martijneken commented Apr 26, 2024

LGTM, but those should be 2 separate PRs.

Sorry, not sure what you mean, this is a minimal PR. I think you're talking about macOS and cargo-raze issues? Yes, those are being addressed separately. Can we get this merged despite the pre-existing CI issues?

Or if you're talking about the buildifier fix, that was a presubmit identified issue and fixing it is trivial and safe.

@PiotrSikora
Copy link
Member

PiotrSikora commented Apr 26, 2024

Or if you're talking about the buildifier fix, that was a presubmit identified issue and fixing it is trivial and safe.

This. I agree that it's trivial and safe, but there is no reason to bundle those 2 unrelated changes into a single PR.

@martijneken
Copy link
Contributor Author

martijneken commented Apr 27, 2024

Split the BUILD fix to #394, PTAL @PiotrSikora

Signed-off-by: Martijn Stevenson <[email protected]>
@PiotrSikora PiotrSikora changed the title fix: Update boringssl to match Envoy (2022-02-07 -> 2023-08-28) Update BoringSSL to 2023-08-28. Apr 29, 2024
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

LGTM, although the fact that we're updating to a version from 8 months ago is a bit questionable.

@PiotrSikora
Copy link
Member

@martijneken sorry, I keep forgetting that for some reason GitHub decides to use my old @google.com email address in UI-triggered merge commits... Could you rebase this on top of master and drop my merge commit? Thanks!

@martijneken martijneken requested review from mpwarres and removed request for mpwarres April 29, 2024 05:27
@martijneken
Copy link
Contributor Author

@PiotrSikora I'm confused why your review is not considered a sufficient Code Owner review. Maybe because you added a merge commit? BTW this triggered a CLA check issue -- are you able to resolve that?

@PiotrSikora
Copy link
Member

@PiotrSikora I'm confused why your review is not considered a sufficient Code Owner review. Maybe because you added a merge commit?

That's not it, see: #392 (comment)

BTW this triggered a CLA check issue -- are you able to resolve that?

Done... But could you merge the master branch to enable merge?

@martijneken martijneken merged commit 04dfb94 into proxy-wasm:master Apr 29, 2024
26 of 31 checks passed
@martijneken martijneken deleted the update-boringssl branch April 29, 2024 18:06
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