slachiewicz commented on PR #3944:
URL: https://github.com/apache/thrift/pull/3944#issuecomment-5935722139
@fishy Copilot raised two points about checkouts that ran `make` before this
change (fresh clones and CI are not affected):
1. **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`.
2. **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:
```diff
--- 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
