Skip to content

[SPARK-58439][BUILD][INFRA] Add dedicated validation script for distributions - #57645

Open
nchammas wants to merge 4 commits into
apache:masterfrom
nchammas:dist-tests
Open

[SPARK-58439][BUILD][INFRA] Add dedicated validation script for distributions#57645
nchammas wants to merge 4 commits into
apache:masterfrom
nchammas:dist-tests

Conversation

@nchammas

@nchammasnchammas commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add a new Python script, dev/validate-distribution, that runs at the end of dev/make-distribution.sh and checks several basic things about the distribution.

The checks that run depend on the flags that are passed to make-distribution.sh. If a Python package is built, Python-specific tests are run; if an R package is built, R-specific tests are run; etc. I used the same command line flags in the validation script as in the build script.

Why are the changes needed?

This extends the work done in #56453 and checks additional, basic things that would likely fail a release if done incorrectly somehow. Since the script is in Python, it's also a bit easier to maintain.

This will also make it easier to add more checks in the future, should they become relevant. I would add smoke tests for the new CLI, for example, if #57313 is accepted.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Here's what the output looks like when the distribution is fine:

$ ./dev/validate-distribution dist/
Testing distribution at: .../spark/dist
Checking distribution structure... OK
Checking bin/spark-submit --version... OK
Checking bin/spark-shell --version... OK
Checking bin/pyspark --version... OK
Checking bin/spark-sql --version... OK
Checking sbin/start-thriftserver.sh --help... OK
Checking sbin/start-connect-server.sh --help... OK
All distribution validation checks passed.

And here's what it looks like when the distribution has some problems:

$ ./dev/validate-distribution dist/ --pip /tmp/fake-pip
Testing distribution at: .../spark/dist
Checking distribution structure... FAILED
MISSING: LICENSE
MISSING: RELEASE
Checking bin/spark-submit --version... OK
Checking bin/spark-shell --version... OK
Checking bin/pyspark --version... FAILED
Checking bin/spark-sql --version... OK
Checking sbin/start-thriftserver.sh --help... OK
Checking sbin/start-connect-server.sh --help... OK
Checking pyspark-fake.tar.gz for LICENSE and NOTICE... FAILED
pyspark-fake.tar.gz: missing LICENSE at the package root
pyspark-fake.tar.gz: missing NOTICE at the package root
5 check(s) failed:
MISSING: LICENSE
MISSING: RELEASE
bin/pyspark --version: [Errno 13] Permission denied: '.../spark/dist/bin/pyspark'
pyspark-fake.tar.gz: missing LICENSE at the package root
pyspark-fake.tar.gz: missing NOTICE at the package root

Was this patch authored or co-authored using generative AI tooling?

I wrote this with assistance from GitHub Copilot.

@nchammas

Copy link
Copy Markdown
ContributorAuthor

@huaxingao - Helpful or overkill?

Comment threaddev/run-distribution-tests Outdated

@HyukjinKwonHyukjinKwon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 blocking, 0 non-blocking, 0 nits.
Useful, well-scoped build-validation tooling; robust and correctly wired.

Verification

Confirmed make-distribution.sh assembles the command as a bash array conditional on MAKE_PIP/MAKE_R/MAKE_TGZ/MAKE_SPARK_CONNECT (validates only what was built; array form avoids quoting bugs). The script accumulates errors and sys.exit(1)s so a validation failure fails the build, and the smoke-test subprocess calls use a timeout with TimeoutExpired handling (the one reviewer concern, already fixed).

@pan3793

Copy link
Copy Markdown
Member

I personally feel this is overkill ... leave it to others to make a decision.

not a big deal, but this seems to add a hard dependency on Python to the make-distribution.sh script even without --pip.

gaogaotiantian pushed a commit that referenced this pull request Aug 15, 2026
…up.py files
### What changes were proposed in this pull request?
Move various include commands from MANIFEST.in into the various setup.py files. MANIFEST.in `recursive-include ... *` is recursive, whereas `*` in setuptools isn't. The equivalent in setuptools is to use `**`.
Eliminate some of the setup.py hacks that were implemented to work around MANIFEST.in over-packaging files.
### Why are the changes needed?
Having duplicate (and unnecessary!) packaging specs across setup.py and MANIFEST.in is [confusing]. MANIFEST.in was also triggering more files to be included than desired, which required some hacks to work around. With this cleanup, some of those hacks are no longer necessary.
The new addition of an explicit `license_files` argument is not necessary -- setuptools packages these files by default -- but nice to have since it's explicit.
[confusing]: #57763 (comment)
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
I built the three sdists off of master and then again from this branch. I diffed their entire contents, both files and file contents, and found no differences except in the MANIFEST.in and setup.py that we changed here, which is expected.
I also repeated this test but by calling `./dev/make-distribution.sh --pip` instead of `python ... sdist`, since `make-distribution.sh` stages license and JAR files before building the sdist. Again, no differences except what we expect in MANIFEST.in and setup.py.
Here are the test scripts I used:
- [sdist-diff.sh](https://github.com/user-attachments/files/31071343/sdist-diff.sh)
- [sdist-diff-make-dist.sh](https://github.com/user-attachments/files/31071338/sdist-diff-make-dist.sh)
Our CI packaging tests are a bit slim. I've separately proposed #57645 to make them more comprehensive.
### Was this patch authored or co-authored using generative AI tooling?
I wrote the test scripts with assistance from GitHub Copilot.
Closes#57973 from nchammas/pyspark-manifest-cleanup.
Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: Tian Gao <gaogaotiantian@hotmail.com>
gaogaotiantian pushed a commit that referenced this pull request Aug 15, 2026
…up.py files
### What changes were proposed in this pull request?
Move various include commands from MANIFEST.in into the various setup.py files. MANIFEST.in `recursive-include ... *` is recursive, whereas `*` in setuptools isn't. The equivalent in setuptools is to use `**`.
Eliminate some of the setup.py hacks that were implemented to work around MANIFEST.in over-packaging files.
### Why are the changes needed?
Having duplicate (and unnecessary!) packaging specs across setup.py and MANIFEST.in is [confusing]. MANIFEST.in was also triggering more files to be included than desired, which required some hacks to work around. With this cleanup, some of those hacks are no longer necessary.
The new addition of an explicit `license_files` argument is not necessary -- setuptools packages these files by default -- but nice to have since it's explicit.
[confusing]: #57763 (comment)
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
I built the three sdists off of master and then again from this branch. I diffed their entire contents, both files and file contents, and found no differences except in the MANIFEST.in and setup.py that we changed here, which is expected.
I also repeated this test but by calling `./dev/make-distribution.sh --pip` instead of `python ... sdist`, since `make-distribution.sh` stages license and JAR files before building the sdist. Again, no differences except what we expect in MANIFEST.in and setup.py.
Here are the test scripts I used:
- [sdist-diff.sh](https://github.com/user-attachments/files/31071343/sdist-diff.sh)
- [sdist-diff-make-dist.sh](https://github.com/user-attachments/files/31071338/sdist-diff-make-dist.sh)
Our CI packaging tests are a bit slim. I've separately proposed #57645 to make them more comprehensive.
### Was this patch authored or co-authored using generative AI tooling?
I wrote the test scripts with assistance from GitHub Copilot.
Closes#57973 from nchammas/pyspark-manifest-cleanup.
Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: Tian Gao <gaogaotiantian@hotmail.com>
(cherry picked from commit dc15294)
Signed-off-by: Tian Gao <gaogaotiantian@hotmail.com>
@nchammas

Copy link
Copy Markdown
ContributorAuthor

@pan3793 - I added a more complete smoke test that catches the kind of problem documented in #58428. ;)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nchammas@pan3793@HyukjinKwon@uros-b