tuhaihe commented on PR #1383:
URL: https://github.com/apache/cloudberry/pull/1383#issuecomment-5673914984
I have repushed this PR with a different approach: instead of relocating the
yyjson submodule, it removes the submodule and fetches yyjson at configure
time, as @edespino suggested above. Thanks Ed — your `fetchcontent-yyjson`
branch is what this is based on.
The reason for the change of direction is the 2.2.0 release.
## What prompted this
During the 2.2.0-rc1 IPMC vote, Justin Mclean raised several LICENSE
completeness issues, one of them about yyjson: he noted that
`dependency/yyjson/doc/doxygen/html/jquery.js` bundles jQuery, Sizzle,
jQuery UI and jQuery UI Touch Punch, plus doxygen-awesome-css, while our
LICENSE entry points only at yyjson's own MIT license.
That is accurate, and there is slightly more of it than reported. The
generated doxygen output under `dependency/yyjson/doc/` carries five groups
of third-party code that the root LICENSE does not account for:
| Component | License |
| --- | --- |
| jQuery 3.6.0 (Sizzle is inlined into it) | MIT, OpenJS Foundation |
| jQuery UI 1.12.1 | MIT, jQuery Foundation |
| jQuery UI Touch Punch 0.2.3 | dual MIT / GPLv2 |
| doxygen-awesome-css | MIT, jothepro |
| Doxygen's own helper scripts (`dynsections.js`, `navtree.js`, `resize.js`,
`menu.js`) | MIT, Dimitri van Heesch |
The last group was not in Justin's list. It is worth noting that these are
MIT and not GPL: Doxygen itself is GPLv2, but it licenses the JavaScript it
emits under MIT via an explicit `@licstart` block, so there is no Category X
problem here — only a completeness one.
We could declare all five in the root LICENSE. But before doing that it is
worth asking whether we need to carry yyjson in the source tree at all.
## yyjson is not built by anything we ship
This also answers the question @jiaqizho raised earlier in this thread:
> The reason why `dependency/yyjson` is separated and stored instead of being
> placed under PAX is because not only PAX depends on `yyjson`. Does the core
> of cloud also rely on `yyjson`?
It does not. Searching `src/`, `contrib/`, `gpcontrib/`, `gpMgmt/` and
`gpAux/`, exactly one file references yyjson:
```
contrib/pax_storage/src/cpp/manifest/manifest.c:42:#include "yyjson.h"
```
There is no second consumer, in PAX or anywhere else.
Beyond that, even PAX does not compile it in any configuration we build.
`contrib/pax_storage/CMakeLists.txt` only pulls yyjson in under
`if(USE_MANIFEST_API AND NOT USE_PAX_CATALOG)`. `USE_MANIFEST_API` defaults
to `OFF` and `USE_PAX_CATALOG` defaults to `ON`, and nothing in the tree
turns them the other way — not CI, not the devops build scripts, not
configure. They can only be set by hand through the environment.
Checked against an actual release-configuration build on Rocky 9:
- no yyjson objects or `libyyjson*` anywhere in the build tree
- no `libyyjson*` in the installed `$GPHOME`
- `pax.so` and `libpaxformat.so` contain **0** yyjson symbols
- `manifest.c` produces no object file at all
- the build log confirms `build pax, USE_MANIFEST_API=OFF USE_PAX_CATALOG=ON`
So we ship a submodule that no released binary contains.
## What it costs to keep it
- `dependency/yyjson` is 9.3 MB across 804 files
- of which 3.0 MB and 201 files are generated doxygen output
- `pom.xml` carries **249** Apache RAT exclusions for it, **200** of them for
the docs alone
- plus the five undeclared third-party components above
## The change
Fetching on demand removes the components rather than documenting them,
which seems clearly better for a dependency that no shipped build uses.
This also matches what we already do elsewhere in the tree: psutil, PyYAML
and PyGreSQL are downloaded during the build by `gpMgmt/bin/Makefile`, with
the comment "This replaces the previously bundled tar.gz files to comply with
Apache Release policy". yyjson is the same kind of problem with the same kind
of answer.
The version is now a release tag rather than a submodule pointer. Ed asked
earlier why the previous commit SHA was chosen — as far as I can tell there
was no particular reason: `61c03f6` is "Try fix github action", not a release
at all, on a tree whose `CMakeLists.txt` still said `VERSION 0.10.0`.
Removed along with the submodule: the `LICENSE` entry,
`licenses/LICENSE-yyjson.txt`, and all 249 RAT exclusions — 285 deletions
against 29 insertions, on top of the 9.3 MB the submodule itself brought in.
### One deviation from Ed's draft
Ed's branch tries a system yyjson through `find_package(yyjson QUIET)` before
falling back to fetching. I left that path out, because as written it cannot
work:
- on the `find_package` path, `yyjson_SOURCE_DIR` is never set, so the
include
path in `pax.cmake` would expand to `/src`
- an installed yyjson exports the target as `yyjson::yyjson`, but
`pax.cmake` links the bare name `yyjson`
None of our build images can reach that branch either (yyjson is not in
Rocky's default repositories, and not in Ubuntu 22.04), so it would never be
exercised. Since the previous behaviour was always to build yyjson from
source, always fetching keeps this change behaviour-preserving. Happy to add
the system-package path back properly — via a `yyjson::yyjson` alias and the
target's `INTERFACE_INCLUDE_DIRECTORIES` — if people would rather have it.
## Verification
On Rocky 9:
- **Default configuration** — the PAX build succeeds with `http_proxy` and
`https_proxy` unset, i.e. with no network access at all. This is the
configuration every release build uses, and it is unaffected.
- **`USE_MANIFEST_API=ON USE_PAX_CATALOG=OFF`** — CMake reports
`Fetching yyjson 0.12.0 for the manifest API`, `_deps/yyjson-src` checks
out
at tag `0.12.0`, `libyyjson.so.0.12.0` is built, `flags.make` puts
`-I.../_deps/yyjson-src/src` on the compile line that `manifest.c` uses,
and
`link.txt` carries `libyyjson.so.0.12.0`.
## A separate finding for the PAX maintainers
While testing the second configuration I found that it does not build on
`main` at all, with or without this PR:
```
micro_partition_iterator_manifest.cc:309:30: error: 'struct RelationData' has
no member named 'rd_node'; did you mean 'rd_id'?
```
That is the PostgreSQL 16 rename of `rd_node` to `rd_locator`, which this
file
never picked up. An unpatched tree with the submodule still in place fails at
the same two lines with the same error, so this is pre-existing and unrelated
to the change here.
Fixing it is more than those two lines — `pax_access_method_internal.cc` and
`catalog/manifest_api.h` still use `RelFileNode` in their `USE_MANIFEST_API`
branches as well. I will open that separately rather than mix it into this
PR,
since it applies only to the PostgreSQL 16 based `main` branch. Flagging it
here mainly as a data point: the manifest API path has been unbuildable since
the PostgreSQL 16 merge, which is consistent with nothing enabling it.
cc @jiaqizho @leborchuk
## Backpatch
2.2.0 is cut from `REL_2_STABLE`, not `main`, so this needs to land there too
to help the release. I checked the patch against `REL_2_STABLE`: everything
applies cleanly except one hunk in `LICENSE`, because #1987 already corrected
`dependencies/yyjson` to `dependency/yyjson` there. Resolving it just means
deleting those two lines 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
