You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/07/13 03:12:15 UTC

[PR] fix(ci): make the source-release binary-file check actually run [hudi]

wombatu-kun opened a new pull request, #19267:
URL: https://github.com/apache/hudi/pull/19267

   ### Describe the issue this Pull Request addresses
   
   Closes #19266
   
   `scripts/release/validate_source_binary_files.sh` guards the Apache source release against binary files. It has never fired on Linux, which is every CI runner and most release managers' machines, for two independent reasons.
   
   **The flag is BSD-only.** The check pipes every file through `file -I`, which is the macOS spelling of the MIME option. GNU `file` rejects it with `file: invalid option -- 'I'`, prints usage to stderr and nothing to stdout, so `wc -l` counts zero and `numBinaryFiles` is always `0`. `set -o errexit` does not catch this: the pipeline sits inside a command substitution in an assignment, and without `pipefail` the assignment takes the exit status of the last stage (`sed`), which is 0. Running the current script unmodified against a clean checkout of master, whose tree holds 105 images outside `src/test/`, exits 0 and prints `No Binary Files in the source files? - [OK]`.
   
   **The check runs in the wrong directory.** Its contract is "run me from the root of a source release tree" - that is how `validate_staged_release.sh` calls it, after `cd`-ing into the extracted tarball. The `validate-source` job in `bot.yml` runs it against the raw git checkout, which still contains `rfc/`, `docker/images/`, `hudi-notebooks/notebooks/common/images/` and `.idea/`, all of which `create_source_directory.sh` deliberately excludes from the release. So repairing the flag alone would turn CI red on 105 files that are not in the source release at all.
   
   Found while reviewing #19265, which adds the first binary asset that reaches the source tarball outside `release/` and drew no red check.
   
   ### Summary and Changelog
   
   The guard now actually guards, and it can no longer fail silently.
   
   - `scripts/release/validate_source_binary_files.sh`: ask libmagic for the **charset** instead of the MIME type. `file --mime` is the long option for both GNU's `-i` and BSD's `-I`, so it is the one spelling that works on Linux CI and on a release manager's macOS, and its `charset=binary` field answers the question directly. The previous approach - an allowlist of permitted MIME types (`text/`, `application/json`, `application/xml`) - is brittle against libmagic's growing type vocabulary: on `file-5.45` it produces five false positives on master alone, because libmagic now labels the JSON test data `application/x-ndjson` and misdetects `SparkIndexerSupport.java` as `application/javascript`. Empty files (`inode/x-empty`, which libmagic also reports as `charset=binary`) are exempted, and the existing `release/` and `src/test/` exemptions are kept.
   - Same file: add `set -o pipefail`, so a broken `file` invocation fails the job loudly instead of yielding an empty pipeline that reads as "no binary files found". This is the regression guard for the bug itself.
   - Same file: run `file` once over a batched `find -print0 | xargs -0` instead of spawning one `file` process per path with `xargs -I {}`, and build the report from the single pass rather than re-running the whole pipeline to print it. This also removes an inconsistency between the two runs, which excluded `release/` and `release/release_guide` respectively.
   - `.github/workflows/bot.yml`: create the source release directory once, then run **both** the binary check and the copyright check inside it, mirroring what `validate_staged_release.sh` already does with an extracted tarball. The copyright check already ran there; only its `create_source_directory.sh` call moves out into its own step.
   
   Verified locally against a clean checkout of master:
   
   | Scenario | Before | After |
   | --- | --- | --- |
   | Fixed script, source release directory | n/a | exit 0, `[OK]` |
   | Fixed script, raw git checkout | n/a | exit 1, 105 files (100 under `rfc/`, 3 under `hudi-notebooks/`, 1 each under `.idea/` and `docker/`) |
   | Fixed script, a PNG planted in a source path | n/a | exit 1, path named |
   | Fixed script with a deliberately broken `file` flag | n/a | exit 123 |
   | **Current script, tree containing 105 binaries** | **exit 0, `[OK]`** | n/a |
   
   ### Impact
   
   CI and release tooling only. No runtime, public API, config, storage format or user-facing change.
   
   One cross-PR interaction is worth calling out: once this merges, the guard catches `hudi-agent-gateway/src/hudi_agent_gateway/ui/hudi-logo.png` from #19265 (`image/png; charset=binary`), because `create_source_directory.sh` does not exclude that path. That is the guard doing its job, and it is independent of merge order - whichever lands second sees the red check. #19265 can clear it by inlining the logo as SVG or as a data URI in `style.css`, which also removes the need for the `.png` exemption that PR adds to `validate_source_copyright.sh`.
   
   ### Risk Level
   
   low
   
   The change is confined to a release-validation script and the `validate-source` CI job. The new check passes on current master inside the source release directory with zero files flagged, and `pipefail` makes any future breakage loud rather than silent. The one behaviour that gets stricter is the intended one.
   
   ### Documentation Update
   
   none
   
   ### Contributor's checklist
   
   - [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
   - [x] Enough context is provided in the sections above
   - [x] Adequate tests were added if applicable
   


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(ci): make the source-release binary-file check actually run [hudi]

Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19267:
URL: https://github.com/apache/hudi/pull/19267#discussion_r3585826509


##########
scripts/release/validate_source_binary_files.sh:
##########
@@ -19,16 +19,38 @@
 # under the License.
 #
 
-# fail immediately
+# Run this from the root of a source release tree, i.e. the output of
+# create_source_directory.sh (see the validate-source job in bot.yml) or an
+# extracted source tarball (see validate_staged_release.sh). Paths that never
+# reach the source release are excluded by create_source_directory.sh, not here.
+
+# fail immediately; pipefail so that a broken `file` invocation can never again
+# yield an empty pipeline that reads as "no binary files found"
 set -o errexit
 set -o nounset
+set -o pipefail
 
 echo "Checking for binary files in the source files"
-numBinaryFiles=`find . -iname '*' | xargs -I {} file -I {} | grep -va directory | grep -v "release/" | grep -v "/src/test/" | grep -va 'application/json' | grep -va 'text/' | grep -va 'application/xml' | grep -va 'application/json' | wc -l | sed -e s'/ //g'`
 
-if [ "$numBinaryFiles" -gt "0" ]; then
+# `--mime` is the long option for both GNU file's `-i` and BSD file's `-I`, so it
+# is the only spelling that works on Linux CI and on a release manager's macOS.
+# Its charset field answers the question directly: libmagic reports
+# charset=binary for anything that is not text. Matching on the charset rather
+# than on a list of permitted MIME types keeps this robust against libmagic's
+# growing type vocabulary, which already labels the JSON test data
+# application/x-ndjson and misdetects at least one .java source as
+# application/javascript.
+mimeTypes=$(find . -type f -print0 | xargs -0 file --mime)
+
+# An empty file reports as inode/x-empty; charset=binary, and is neither binary nor
+# a licensing concern. release/ holds the release guide's screenshot; src/test/ holds
+# the binary fixtures (parquet, avro, hfile) that tests read.
+binaryFiles=$(echo "$mimeTypes" | grep -a 'charset=binary' | grep -v 'inode/x-empty' \

Review Comment:
   nit: the `-v` stages lack `-a`, so a non-UTF-8 filename can flip a later grep into binary mode. `-a` on all stages (or `LC_ALL=C` up top) keeps the chain byte-safe.



##########
scripts/release/validate_source_binary_files.sh:
##########
@@ -19,16 +19,38 @@
 # under the License.
 #
 
-# fail immediately
+# Run this from the root of a source release tree, i.e. the output of
+# create_source_directory.sh (see the validate-source job in bot.yml) or an
+# extracted source tarball (see validate_staged_release.sh). Paths that never
+# reach the source release are excluded by create_source_directory.sh, not here.
+
+# fail immediately; pipefail so that a broken `file` invocation can never again
+# yield an empty pipeline that reads as "no binary files found"
 set -o errexit
 set -o nounset
+set -o pipefail
 
 echo "Checking for binary files in the source files"
-numBinaryFiles=`find . -iname '*' | xargs -I {} file -I {} | grep -va directory | grep -v "release/" | grep -v "/src/test/" | grep -va 'application/json' | grep -va 'text/' | grep -va 'application/xml' | grep -va 'application/json' | wc -l | sed -e s'/ //g'`
 
-if [ "$numBinaryFiles" -gt "0" ]; then
+# `--mime` is the long option for both GNU file's `-i` and BSD file's `-I`, so it
+# is the only spelling that works on Linux CI and on a release manager's macOS.
+# Its charset field answers the question directly: libmagic reports
+# charset=binary for anything that is not text. Matching on the charset rather
+# than on a list of permitted MIME types keeps this robust against libmagic's
+# growing type vocabulary, which already labels the JSON test data
+# application/x-ndjson and misdetects at least one .java source as
+# application/javascript.
+mimeTypes=$(find . -type f -print0 | xargs -0 file --mime)
+
+# An empty file reports as inode/x-empty; charset=binary, and is neither binary nor
+# a licensing concern. release/ holds the release guide's screenshot; src/test/ holds
+# the binary fixtures (parquet, avro, hfile) that tests read.
+binaryFiles=$(echo "$mimeTypes" | grep -a 'charset=binary' | grep -v 'inode/x-empty' \
+  | grep -v 'release/' | grep -v '/src/test/' || true)

Review Comment:
   nit: substring match - this also exempts any nested `*/release/` path (or a hypothetical `prerelease/`). `grep -v '^\./release/'` pins it to the release-guide dir it is meant for.



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(ci): make the source-release binary-file check actually run [hudi]

Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19267:
URL: https://github.com/apache/hudi/pull/19267


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(ci): make the source-release binary-file check actually run [hudi]

Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19267:
URL: https://github.com/apache/hudi/pull/19267#issuecomment-4954121756

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "53b3cae947f25af4768ca188b7ca31bee9e69b0c",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "53b3cae947f25af4768ca188b7ca31bee9e69b0c",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 53b3cae947f25af4768ca188b7ca31bee9e69b0c UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     @hudi-bot supports the following commands:
   
    - `@hudi-bot run azure` re-run the last Azure build
   </details>


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(ci): make the source-release binary-file check actually run [hudi]

Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #19267:
URL: https://github.com/apache/hudi/pull/19267#discussion_r3586411508


##########
scripts/release/validate_source_binary_files.sh:
##########
@@ -19,16 +19,38 @@
 # under the License.
 #
 
-# fail immediately
+# Run this from the root of a source release tree, i.e. the output of
+# create_source_directory.sh (see the validate-source job in bot.yml) or an
+# extracted source tarball (see validate_staged_release.sh). Paths that never
+# reach the source release are excluded by create_source_directory.sh, not here.
+
+# fail immediately; pipefail so that a broken `file` invocation can never again
+# yield an empty pipeline that reads as "no binary files found"
 set -o errexit
 set -o nounset
+set -o pipefail
 
 echo "Checking for binary files in the source files"
-numBinaryFiles=`find . -iname '*' | xargs -I {} file -I {} | grep -va directory | grep -v "release/" | grep -v "/src/test/" | grep -va 'application/json' | grep -va 'text/' | grep -va 'application/xml' | grep -va 'application/json' | wc -l | sed -e s'/ //g'`
 
-if [ "$numBinaryFiles" -gt "0" ]; then
+# `--mime` is the long option for both GNU file's `-i` and BSD file's `-I`, so it
+# is the only spelling that works on Linux CI and on a release manager's macOS.
+# Its charset field answers the question directly: libmagic reports
+# charset=binary for anything that is not text. Matching on the charset rather
+# than on a list of permitted MIME types keeps this robust against libmagic's
+# growing type vocabulary, which already labels the JSON test data
+# application/x-ndjson and misdetects at least one .java source as
+# application/javascript.
+mimeTypes=$(find . -type f -print0 | xargs -0 file --mime)
+
+# An empty file reports as inode/x-empty; charset=binary, and is neither binary nor
+# a licensing concern. release/ holds the release guide's screenshot; src/test/ holds
+# the binary fixtures (parquet, avro, hfile) that tests read.
+binaryFiles=$(echo "$mimeTypes" | grep -a 'charset=binary' | grep -v 'inode/x-empty' \

Review Comment:
   Done 698df343722b



##########
scripts/release/validate_source_binary_files.sh:
##########
@@ -19,16 +19,38 @@
 # under the License.
 #
 
-# fail immediately
+# Run this from the root of a source release tree, i.e. the output of
+# create_source_directory.sh (see the validate-source job in bot.yml) or an
+# extracted source tarball (see validate_staged_release.sh). Paths that never
+# reach the source release are excluded by create_source_directory.sh, not here.
+
+# fail immediately; pipefail so that a broken `file` invocation can never again
+# yield an empty pipeline that reads as "no binary files found"
 set -o errexit
 set -o nounset
+set -o pipefail
 
 echo "Checking for binary files in the source files"
-numBinaryFiles=`find . -iname '*' | xargs -I {} file -I {} | grep -va directory | grep -v "release/" | grep -v "/src/test/" | grep -va 'application/json' | grep -va 'text/' | grep -va 'application/xml' | grep -va 'application/json' | wc -l | sed -e s'/ //g'`
 
-if [ "$numBinaryFiles" -gt "0" ]; then
+# `--mime` is the long option for both GNU file's `-i` and BSD file's `-I`, so it
+# is the only spelling that works on Linux CI and on a release manager's macOS.
+# Its charset field answers the question directly: libmagic reports
+# charset=binary for anything that is not text. Matching on the charset rather
+# than on a list of permitted MIME types keeps this robust against libmagic's
+# growing type vocabulary, which already labels the JSON test data
+# application/x-ndjson and misdetects at least one .java source as
+# application/javascript.
+mimeTypes=$(find . -type f -print0 | xargs -0 file --mime)
+
+# An empty file reports as inode/x-empty; charset=binary, and is neither binary nor
+# a licensing concern. release/ holds the release guide's screenshot; src/test/ holds
+# the binary fixtures (parquet, avro, hfile) that tests read.
+binaryFiles=$(echo "$mimeTypes" | grep -a 'charset=binary' | grep -v 'inode/x-empty' \
+  | grep -v 'release/' | grep -v '/src/test/' || true)

Review Comment:
   Done 698df343722b



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org