Nishuuzz commented on PR #55162:
URL: https://github.com/apache/arrow/pull/55162#issuecomment-6401695013
You're right, and my description was wrong. Thanks for digging out the
actual history.
I checked 5ca85922ae and the loop was there:
```python
for lang in implementations:
param = f'with_{lang}'
if with_all:
args[param] = with_all
enabled_implementations += args[param]
```
and 9a219256bf dropped it along with `enabled_implementations` when
`select_testers()` took over. So this is a regression in 22.0.0, not a feature
that was never wired up. I should also withdraw the `_set_default` claim in my
description — that helper has been unused since 2019 and has nothing to do with
`--with-all`; I used it because it happened to fit, not because it was left
over from this.
On whether the feature is needed, one thing that I think explains the
silence: nothing in the repository passes `--with-all`. CI goes through
`ARCHERY_INTEGRATION_WITH_*` in `ci/scripts/integration_arrow.sh`,
`verify-release-candidate.sh` passes `--with-cpp=${TEST_INTEGRATION_CPP}`, and
`generate_files_for_endian_test.sh` passes `--with-cpp=1`. The only reference
left is `docs/source/format/Integration.rst`:
```
archery integration --with-all --run-flight --run-ipc --run-c-data
```
So the people who would hit this are contributors following that page
locally, and the failure is a `Need to enable at least one implementation to
test` that looks like their own setup being wrong rather than a broken flag.
That's not the kind of thing that usually turns into a bug report.
Either direction is fine by me and it's your call — I'd just rather the docs
and the code agreed:
- keep it, as in this PR, and the documented command works again;
- or drop `--with-all` entirely, in which case that doc line needs rewriting
with explicit `--with-<lang>` flags. Happy to switch this PR over to that if
you prefer; it's a smaller diff than what's here.
If you do keep it, two differences from the pre-22.0.0 behaviour worth
deciding on, since neither is forced:
1. The old loop assigned unconditionally, so `--with-all --with-java=0`
still enabled Java. Mine lets the explicit value win, which is why the options
moved to `default=None`. I chose that because the help text says "by default",
but matching the old behaviour exactly is a two-line change.
2. The old list was hardcoded, and `--with-ruby` was added after it was
removed, so it would not have been picked up. Mine derives the set from the
`with_*` options, so a new implementation is included automatically. That
seemed worth keeping given a stale hardcoded list is roughly how this got lost
in the first place.
--
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]
