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

Github Actions: Remove --disable-tests #2886

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Conversation

fishy
Copy link
Member

@fishy fishy commented Nov 22, 2023

With --disable-tests, for example for Go the make check under lib/go
would only run unit tests under lib/go/thrift but not the unit tests
under lib/go/test.

Also some changes in lib/go/test/fuzz/Makefile.am so it works in both go
1.20 and 1.21 (The current state breaks in 1.21 but because of
--disable-tests we never noticed that).

@fishy fishy added ci github_actions Pull requests that update GitHub Actions code labels Nov 22, 2023
@fishy fishy force-pushed the go-run-tests branch 3 times, most recently from b5db483 to 4b9be1a Compare November 22, 2023 17:33
@fishy fishy requested review from dcelasun and jimexist November 22, 2023 17:34
@fishy fishy marked this pull request as ready for review November 22, 2023 17:34
@fishy
Copy link
Member Author

fishy commented Nov 22, 2023

cc @catenacyber this changes removes go.mod/go.sum under lib/go/test/fuzz (they are now generated on-the-fly by make check now), which might affect how oss-fuzz runs. based on the previous PR the impact might be minimal but just want to make sure :)

it would be great if oss-fuzz can use go's native fuzz implementation instead.

@fishy fishy added the golang patches related to go language label Nov 22, 2023
@catenacyber
Copy link
Contributor

this changes removes go.mod/go.sum under lib/go/test/fuzz (they are now generated on-the-fly by make check now), which might affect how oss-fuzz runs. based on the previous PR the impact might be minimal but just want to make sure :)

Fails with oss-fuzz
python3 infra/helper.py build_fuzzers thrift with patched Dockerfile

-RUN git clone --depth 1 https://github.com/apache/thrift
+RUN git clone --depth 1 --branch go-run-tests https://github.com/fishy/thrift

it would be great if oss-fuzz can use go's native fuzz implementation instead.

@AdamKorcz this is now the case, right ?
See https://google.github.io/oss-fuzz/getting-started/new-project-guide/go-lang/#native-go-fuzzing-support

@AdamKorcz
Copy link

@fishy
Copy link
Member Author

fishy commented Nov 27, 2023

@catenacyber thanks, can you try again with the newest version of this branch?

With --disable-tests, for example for Go the `make check` under `lib/go`
would only run unit tests under `lib/go/thrift` but not the unit tests
under `lib/go/test`.

Also some changes in lib/go/test/fuzz/Makefile.am so it works in both go
1.20 and 1.21 (The current state breaks in 1.21 but because of
`--disable-tests` we never noticed that).
@catenacyber
Copy link
Contributor

catenacyber thanks, can you try again with the newest version of this branch?

Still failing

First make is failing with make[4]: *** No rule to make target 'src/thrift/numeric_cast.h', needed by 'all-am'. Stop.

Then golang is failing with logs

go: finding module for package github.com/apache/thrift/lib/go/test/fuzz/gen-go/tutorial
go: finding module for package github.com/apache/thrift/lib/go/test/fuzz/gen-go/shared
go: github.com/apache/thrift/lib/go/test/fuzz imports
	github.com/apache/thrift/lib/go/test/fuzz/gen-go/shared: module github.com/apache/thrift/lib/go/test@latest found (v0.0.0-20231127082722-8a6bcc76f30b), but does not contain package github.com/apache/thrift/lib/go/test/fuzz/gen-go/shared
go: github.com/apache/thrift/lib/go/test/fuzz imports
	github.com/apache/thrift/lib/go/test/fuzz/gen-go/tutorial: module github.com/apache/thrift/lib/go/test@latest found (v0.0.0-20231127082722-8a6bcc76f30b), but does not contain package github.com/apache/thrift/lib/go/test/fuzz/gen-go/tutorial
github.com/apache/thrift/lib/go/test/fuzz
github.com/apache/thrift/lib/go/test/fuzz
Running go-fuzz -tags gofuzz -func Fuzz -o fuzz_go_tutorial.a .
fuzz.go:30:2: no required module provides package github.com/apache/thrift/lib/go/test/fuzz/gen-go/shared; to add it:
	go get github.com/apache/thrift/lib/go/test/fuzz/gen-go/shared
fuzz.go:31:2: no required module provides package github.com/apache/thrift/lib/go/test/fuzz/gen-go/tutorial; to add it:
	go get github.com/apache/thrift/lib/go/test/fuzz/gen-go/tutorial

@fishy
Copy link
Member Author

fishy commented Nov 29, 2023

@catenacyber hmm the make failing is weird, certainly not related to this change.

for the rest, can you point me to the script actually run by oss-fuzz? I think some slight modifications are needed. if you look at the changes to files under lib/go/test/fuzz in this PR, we no longer generate individual go modules (run go mod init under the two directories generated by thrift compiler, and changed the import path in fuzz.go. we also changed the thrift compiler args to make it work.

@catenacyber
Copy link
Contributor

@fishy
Copy link
Member Author

fishy commented Nov 29, 2023

https://github.com/google/oss-fuzz/blob/master/projects/thrift/build.sh

Let me know id it needs some changes

@catenacyber:

We would need to change line 32 into:

thrift -r --gen go:package_prefix=github.com/apache/thrift/lib/go/test/fuzz/gen-go/ ../../../../tutorial/tutorial.thrift

and then remove lines 33-34 (line 35 probably can also be removed, but I think leaving it be would also be harmless)

@fishy
Copy link
Member Author

fishy commented Nov 30, 2023

@catenacyber I created google/oss-fuzz#11296 on oss-fuzz.

@fishy fishy merged commit 022d027 into apache:master Dec 4, 2023
17 of 26 checks passed
@fishy fishy deleted the go-run-tests branch December 4, 2023 14:57
@catenacyber
Copy link
Contributor

@Jens-G I see https://issues.apache.org/jira/browse/THRIFT-5660

Does it explain the build failure No rule to make target 'src/thrift/numeric_cast.h', needed by 'all-am'. ?

@Jens-G
Copy link
Member

Jens-G commented Dec 6, 2023

That was added in 6e9cbbd and 779deab. The file was then removed in 49e4cea but as it seems not the reference in the *.am file. So I would say throw it out.

@catenacyber
Copy link
Contributor

That was added in 6e9cbbd and 779deab. The file was then removed in 49e4cea but as it seems not the reference in the *.am file. So I would say throw it out.

Is anyone taking care of this ?

@fishy
Copy link
Member Author

fishy commented Dec 7, 2023

That was added in 6e9cbbd and 779deab. The file was then removed in 49e4cea but as it seems not the reference in the *.am file. So I would say throw it out.

Is anyone taking care of this ?

#2899

@fishy
Copy link
Member Author

fishy commented Dec 8, 2023

@catenacyber that is merged.

fishy added a commit to fishy/oss-fuzz that referenced this pull request Dec 15, 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.

[1]: apache/thrift#2886
DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Jan 9, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code golang patches related to go language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants