MisterRaindrop commented on code in PR #1842:
URL: https://github.com/apache/cloudberry/pull/1842#discussion_r3774391929
##########
contrib/Makefile:
##########
@@ -25,6 +25,7 @@ SUBDIRS = \
btree_gin \
btree_gist \
citext \
+ datalake_fdw \
Review Comment:
Good call -- done in 6f40dd0f7db, following the PAX mechanism:
`PGAC_ARG_BOOL` defaulting to `no`, substituted into `Makefile.global`, and a
conditional in `contrib/Makefile` that moves the directory to `ALWAYS_SUBDIRS`
when disabled so the clean targets still reach it.
Three things I decided while doing it, all cheap to reverse if you disagree:
- **The option is spelled `--enable-datalake-fdw`, with dashes.** Every
option in this tree uses dashes (`--enable-tap-tests`, `--enable-ic-udp2`,
`--enable-catalog-ext`), and autoconf maps the dashes to `enable_datalake_fdw`
for the Makefile variable, so the directory name still matches. Happy to use
the underscore form if you would rather it read exactly like the directory.
- **No `AC_DEFINE`.** PAX and ic-udp2 define a macro because C code tests
it; nothing here does, and adding an unread macro would mean touching
`src/include/pg_config.h.in` for no reader. `--enable-pxf` and
`--enable-orafce` already use this shorter four-argument form.
- **`--enable-datalake-fdw` is also added to
`devops/build/automation/cloudberry/scripts/configure-cloudberry.sh`**, next to
`--enable-pax`. Without it the `ic-datalake-fdw` CI job would install nothing
and silently test nothing. I deliberately did *not* add it to `coverity.yml`
and `sonarqube.yml`, which also pass `--enable-pax` -- say the word if you want
the module in the weekly scans too.
**One thing you should know about `configure`.** I regenerated it by hand
rather than running autoconf over the whole file, because the committed
`configure` and `configure.ac` are currently out of sync in *both* directions:
- `configure.ac` has a restructured PAX liburing check (the `case $host_os
in linux*)` block with the macOS fallback comment) that was never regenerated
into `configure`;
- `configure` has a Darwin python shared-library lookup (trying `.dylib` in
addition to `DLSUFFIX`) that does not exist in `configure.ac`.
A full `autoconf` run therefore produced twelve hunks, only four of which
were mine; taking all of them would have swept those two unrelated changes into
this PR and, in the python case, reverted working behaviour. So I applied only
the four hunks for this option and left the drift alone. It is worth someone
reconciling those two separately -- I did not want to do it inside this PR.
Verified by configuring a real tree both ways, not by inspection:
| | configure says | `Makefile.global` | `SUBDIRS` | `ALWAYS_SUBDIRS` |
|---|---|---|---|---|
| `--enable-datalake-fdw` | `... yes` | `enable_datalake_fdw = yes` | has
`datalake_fdw` | -- |
| default | `... no` | `enable_datalake_fdw = no` | -- | has `datalake_fdw` |
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
