Hi Justin, please see the inline comments. Justin Mclean <jus...@classsoftware.com> 于2024年9月1日周日 19:39写道:
> Hi, > > -1 (binding) as some files are misisng incubating in their names, but if > those files are renamed, then +1 (binding) > > Several LICENSE and NOTICE files need updating, but that’s OK due to the > WIP disclaimer. > > Testing on mocOS I checked: > - Not all artifact names include incubating in them, e.g. > apache_gravitino-0.6.0.tar.gz. This needs to be fixed, but there is no need > for a new RC or vote, just rename the files. > For Python release "apache_gravitino-0.6.0.tar.gz" version, as far as I know, Python doesn't support version like "0.6.0-incubating", so I only keep the 0.6.0 delibrately. I also refer to some other incubating projects like HugeGraph (https://pypi.org/project/hugegraph-python/#history), seems they also don't add the "incubating" in their version. > - Signatures and hashes are fine. > - DISCLAIMER (WIP), NOTICE and LICENSE exist in all artifacts. > - The LICENSE and NOTICE is incorrect for apache_gravitino-0.6.0.tar.gz, > same with the licenses directory. The LICENSE and NOTICE files need to > reflect what is in the release artifact, and it seems the binary LICENSE > and NOTICE files for the Java release have been copied here. In this case > of the stock LICENSE file, adding "MIT License Copyright (c) 2016 Dhamu” is > all that is needed. BTW do we know what project this code comes from? > Can you please help to work on this, I don't think I have enough time to figure out all these version things one by one. > - Similarly, the LICENSE and NOTICE files for > gravitino-iceberg-rest-server-0.6.0-incubating-bin and > gravitino-trino-connector-0.6.0-incubating need to be updated to reflect > what is in them. > Also for this, please help on this. > - The main gravitino LICENSE is missing a license for TCPDS. I couldn't > find out this data was licenced from a quick search. Do we know how the > data is licensed? > TPCDS data is generated by its tool, they're the random data (will be varied in each execution). I don't find the license of TPC-DS tool, and not sure about the data if it is OK with ASF license. > - all ASF files have the correct header, although perhaps it could be > added to some XML files. > Can you please create an issue so we can add this? > - No unexpected binary files in the source release > - Compile fails due to rat issues, but can compile from source (and test > pass) with the addition of -x rat > The local compile works for me. I'm not so sure about which file has the issues. > > Other minor issues: > - The KEY file needs to be in > https://dist.apache.org/repos/dist/release/incubator/gravitino/ It will be there when we have an official release. > > - I think gravitino-trino-connector-0.6.0-incubating.tar.gz should include > a “-bin” in the name as it’s not source code. Again, this can be renamed > without a new RC or vote. > Typically for trino connector, we don't add "-bin" in the name as well as other connectors, it's a bit stranger as it is only a plugin, not a standalone service. > - Is the source code for gravitino-trino-connector-0.6.0-incubating and > gravitino-iceberg-rest-server-0.6.0-incubating included in the release? (I > am asking this in case this comes up in the IPMC vote). > They're all included in the Gravitino src package. > - The rat test fails when building the source. The files it fails on are > acceptable, except for possibly some XML files. > Please point out the problems and create an issue. I don't see the issue in my local build. > - Do the “.github” and “.gitattributes” files need to be in the source > release? > I think they're also part of the source code, they can be shipped with source release. If it is not required, we can do this in the next release. > - The Python client README.md build instruction needs to be updated as > they assume it part of the main gravitino repo > > Python README.md already reflects the Python things including how to build Python package, can you please point out what specific issue it is? > > Kind Regards, > Justin