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

Update thrift build script #11296

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

fishy
Copy link
Contributor

@fishy fishy commented Nov 30, 2023

This needs to be merged after 1 to keep thrift working.

1 is needed because the current test code of thrift is actually broken with go 1.21, in order to fix the breakage some changes to go modules are needed, and the same changes need to be applied here as well to keep oss-fuzz working with thrift.

Copy link

fishy is a new contributor to projects/thrift. The PR must be approved by known contributors before it can be merged. The past contributors are: catenacyber

@DavidKorczynski DavidKorczynski marked this pull request as draft December 1, 2023 12:37
@DavidKorczynski
Copy link
Collaborator

This needs to be merged after apache/thrift#2886 to keep thrift working.

Converting this to draft while we're waiting for this

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Good for me.

But there may be another issue with the build in the previous make command

@fishy
Copy link
Contributor Author

fishy commented Dec 4, 2023

apache/thrift#2886 is merged now.

@catenacyber
Copy link
Contributor

hey @DavidKorczynski could you approve to run CI workflow ?

@DavidKorczynski
Copy link
Collaborator

hey @DavidKorczynski could you approve to run CI workflow ?

Done!

@catenacyber
Copy link
Contributor

@fishy could you rebase and force-push now that apache/thrift#2899 was merged ?
So we will check the build is indeed fixed

@fishy
Copy link
Contributor Author

fishy commented Dec 12, 2023

@fishy could you rebase and force-push now that apache/thrift#2899 was merged ?
So we will check the build is indeed fixed

will do that on Friday, currently on vacation without laptop.

This needs to be merged _after_ [1] to keep thrift working.

[1] is needed because the current test code of thrift is actually broken
with go 1.21, in order to fix the breakage some changes to go modules
are needed, and the same changes need to be applied here as well to keep
oss-fuzz working with thrift.

[1]: apache/thrift#2886
@fishy
Copy link
Contributor Author

fishy commented Dec 15, 2023

@catenacyber done.

@fishy fishy marked this pull request as ready for review December 18, 2023 19:18
@DavidKorczynski DavidKorczynski merged commit e81dea9 into google:master Jan 9, 2024
15 checks passed
@fishy fishy deleted the update-thrift branch January 9, 2024 21:28
@catenacyber
Copy link
Contributor

And build fixed :-)

@cfriedt
Copy link

cfriedt commented Nov 2, 2024

@fishy - can you elaborate a bit more on what was broken here that needed to be fixed? I would like to retry removing substituting something for boost::numeric_cast again, if possible, and would like to make sure that nothing was missing.

@fishy
Copy link
Contributor Author

fishy commented Nov 2, 2024

@fishy - can you elaborate a bit more on what was broken here that needed to be fixed? I would like to retry removing substituting something for boost::numeric_cast again, if possible, and would like to make sure that nothing was missing.

@cfriedt The oss-fuzz directory under thrift (https://github.com/apache/thrift/tree/master/lib/go/test/fuzz) first runs thrift compiler to generate 2 go libraries, and the actual test code depends on these 2 go libraries. We used to (before this PR) generate 2 go modules from these 2 go libraries for the test code to use, but this PR (and the thrift PR) changed them to be just part of the go module under lib/go/test/fuzz instead.

@cfriedt
Copy link

cfriedt commented Nov 2, 2024

@fishy - even with the header added to Makefile.am, were there fuzz tests failing? Would be good to reproduce the expected results with my changes.

@fishy
Copy link
Contributor Author

fishy commented Nov 2, 2024

@cfriedt oh I never knew where to check the states/results of oss-fuzz. I just asked @catenacyber to check them for me.

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