THRIFT-6348: Go tests: stop writing ThriftTest.thrift copies into the source tree by slachiewicz · Pull Request #3944 · apache/thrift

slachiewicz · 2026-09-23T06:28:11Z

lib/go/test and test/go wrote copies of test/ThriftTest.thrift into the source tree, hidden by .gitignore. The filtered copy in lib/go/test now goes to a dedicated build/ directory, found with -I build and removed by make clean, and test/go compiles test/ThriftTest.thrift directly. The two .gitignore entries for the in-tree copies are replaced by one for lib/go/test/build/.

Verified: the old and new IncludesTest.thrift recipes → identical gopath/src (20 files).

Did you create an Apache Jira ticket? THRIFT-6348
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit?
Did you do your best to avoid breaking changes? No.
Code change, so the commit carries no CI-skip marker.
No new test: build change; the lib-go CI job runs both rules (make -C lib/go check, make -C test/go check).
AI-assisted: the commit carries a Co-Authored-By: trailer. No new dependency or third-party code.

fishy · 2026-09-23T15:53:14Z

I'm not sure putting .thrift files into gopath is a good idea. Is there any particular issue with the current approach you are trying to fix, besides one more .gitignore'd file?

slachiewicz · 2026-09-24T17:30:23Z

Hi @fishy,

Thanks for the review!

The motivation behind THRIFT-6348 is twofold:

State-dependent IDL resolution & tree sweeping: IncludesTest.thrift and NamespacedTest.thrift have include "ThriftTest.thrift". Before make check is run, lib/go/test/ThriftTest.thrift does not exist in the source tree, so IDEs, language servers, or any tree-wide .thrift sweeps fail to resolve the include. Once make check runs, it leaves a generated/ephemeral IDL copy sitting directly in the source directory.
Redundant copy in test/go: test/go/Makefile.am was doing an exact cp $(THRIFTTEST) ThriftTest.thrift with zero edits, which is completely unnecessary since $(THRIFTCMD) can compile $(THRIFTTEST) directly.

Regarding placing it in gopath/: I placed the filtered copy under gopath/ simply because gopath/ is already created and removed by clean-local: $(RM) -r gopath.

If you prefer keeping gopath/ strictly for Go packages, I can easily write the filtered copy to a dedicated directory like build/ThriftTest.thrift or gen-thrift/ThriftTest.thrift (and pass -I build) instead.

Would you prefer a dedicated build/ directory for the filtered file, or do you have a preferred location in mind?

fishy · 2026-09-24T17:33:59Z

a dedicated directory would be better. gopath is supposed to be a directory managed by go toolchain.

build/ works for me.

slachiewicz · 2026-10-01T05:33:55Z

@fishy moved the filtered copy to a dedicated lib/go/test/build/ directory in 33696ef; gopath no longer holds any .thrift file.

Copilot

Copilot review overview

🟡 Changes recommended

Legacy generated files can shadow the new build input or remain after cleanup.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Remove stale ThriftTest.thrift before compiler invocation · New
Preserve cleanup of legacy generated ThriftTest.thrift · New

What changed in this PR

Moves Go test IDL generation out of the source tree.

Changes:

Compiles the shared Go test IDL directly.
Generates filtered IDL under lib/go/test/build/.
Updates cleanup and ignore rules.

File	Description
`.gitignore`	Replaces obsolete generated-IDL exclusions.
`lib/go/test/Makefile.am`	Generates filtered IDL in `build/`.
`test/go/Makefile.am`	Compiles the canonical IDL directly.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

slachiewicz · 2026-10-01T16:25:10Z

@fishy Copilot raised two points about checkouts that ran make before this change (fresh clones and CI are not affected):

Stale copy can shadow the new input. The compiler resolves a relative include next to the including file before the -I paths (compiler/cpp/src/thrift/main.cc, include path search), so a leftover lib/go/test/ThriftTest.thrift from an older build would be picked up by IncludesTest.thrift instead of build/ThriftTest.thrift.
Cleanup of the old copies is gone. With ThriftTest.thrift dropped from clean-local (and from .gitignore), those leftovers show up in git status and make clean no longer removes them.

A possible fix, kept small:

--- a/lib/go/test/Makefile.am
+++ b/lib/go/test/Makefile.am
 	mkdir -p gopath/src build
+	# Older builds generated ThriftTest.thrift here. The compiler resolves an
+	# include next to the including file before the -I paths, so a leftover
+	# copy would shadow build/ThriftTest.thrift.
+	$(RM) ThriftTest.thrift
 	grep -v 'set<Insanity>' $(THRIFTTEST) > build/ThriftTest.thrift
@@
 clean-local:
-	$(RM) -r gopath build gen-go
+	$(RM) -r gopath build gen-go ThriftTest.thrift
--- a/test/go/Makefile.am
+++ b/test/go/Makefile.am
 clean-local:
-	$(RM) -r src/gen src/github.com/golang src/thrift bin pkg gopath
+	$(RM) -r src/gen src/github.com/golang src/thrift bin pkg gopath ThriftTest.thrift

Which direction do you prefer?

add this transitional cleanup to the PR, or
keep the PR as it is and rely on a one-time git clean for existing checkouts?

Happy to go either way.

fishy · 2026-10-01T16:41:28Z

the current make clean already would clean up ThriftTest.thrift:

$ make clean
rm -rf .libs _libs
rm -f -r gopath ThriftTest.thrift gen-go
rm -f *.lo

so I think that's good enough as-is?

slachiewicz · 2026-10-01T17:48:06Z

Agreed — keeping it as-is, then. Thanks!

Jens-G · 2026-10-13T23:47:32Z

This branch has conflicts that must be resolved.

Jens-G · 2026-10-18T16:43:10Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

Jens-G · 2026-10-19T07:06:08Z

@slachiewicz This branch has conflicts that must be resolved

… source tree Client: go lib/go/test wrote a filtered copy next to the sources and test/go a plain one; both were gitignored, so a checkout's IDL set depended on whether a build had run. Write the filtered copy under build/ and resolve the include with -I; use test/ThriftTest.thrift directly in test/go. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

slachiewicz requested a review from fishy as a code owner September 23, 2026 06:28

mergeable Bot added golang Pull requests that update Go code build and general CI cmake, automake and build system changes labels Sep 23, 2026

slachiewicz marked this pull request as draft September 27, 2026 13:30

slachiewicz force-pushed the THRIFT-6348 branch from 35fd4b4 to 33696ef Compare October 1, 2026 05:33

fishy approved these changes Oct 1, 2026

View reviewed changes

slachiewicz marked this pull request as ready for review October 1, 2026 15:55

Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:55

Copilot started reviewing on behalf of slachiewicz October 1, 2026 15:58

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread lib/go/test/Makefile.am Resolved

Comment thread test/go/Makefile.am Resolved

slachiewicz force-pushed the THRIFT-6348 branch from 33696ef to f2a00fa Compare October 19, 2026 23:24

Copilot AI balanced review requested due to automatic review settings October 19, 2026 23:24

Copilot AI reviewed Oct 19, 2026

Jens-G merged commit a9d4626 into apache:master Oct 20, 2026
109 checks passed

slachiewicz deleted the THRIFT-6348 branch October 28, 2026 00:25

Conversation

slachiewicz commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fishy commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slachiewicz commented Sep 24, 2026

Uh oh!

fishy commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slachiewicz commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

slachiewicz commented Oct 1, 2026

Uh oh!

fishy commented Oct 1, 2026

Uh oh!

slachiewicz commented Oct 1, 2026

Uh oh!

Jens-G commented Oct 13, 2026

Uh oh!

Jens-G commented Oct 18, 2026

Code review

Uh oh!

Jens-G commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

slachiewicz commented Sep 23, 2026 •

edited

Loading

fishy commented Sep 23, 2026 •

edited

Loading

fishy commented Sep 24, 2026 •

edited

Loading

slachiewicz commented Oct 1, 2026 •

edited

Loading

Jens-G commented Oct 19, 2026 •

edited

Loading