[FLINK-33607][build]: Add checksum verification for Maven wrapper
What is the purpose of the change
JIRA: FLINK-33503
FLINK-33503 enabled us to add checksum checks for the Maven wrapper binaries along the update from 3.1.0 to 3.2.0.
But there seems to be an issue with verifying the wrapper's checksum under windows (see MVRAPPER-103). After MVRAPPER-103 is resolved in 3.3.0, we should upgrade Maven wrapper to 3.3.0 or later to enable the feature to verify the wrapper's checksum.
This PR did:
- Upgrade Maven wrapper to the latest 3.3.2 version to include more bug fixes.
- Update the
wrapperSha256Sumformaven-wrapper-3.3.2.jar - Update the mvnw auto-generated script in v3.3.2.
Brief change log
- Upgrade Maven wrapper to the latest 3.3.2 version to include more bug fixes.
- Update the
wrapperSha256Sumformaven-wrapper-3.3.2.jar - Update the mvnw auto-generated script in v3.3.2.
Verifying this change
After the change, maven build runs successfully.
Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
@Public(Evolving): no - The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? no
CI report:
- 7c6f9a2fba68550785dcf100b1b2f7d305b9d221 Azure: SUCCESS
Bot commands
The @flinkbot bot supports the following commands:@flinkbot run azurere-run the last Azure build
@XComp , please help review when available. Thanks.
@snuyanzin @JingGe @XComp , please take a look when available. This PR is doing similar things as https://github.com/apache/flink/pull/23766 , just bump the Maven wrapper version to allow checksum verification. Thanks.
@snuyanzin @JingGe @XComp , please take a look when available. This PR is doing similar things as https://github.com/apache/flink/pull/23766 , just bump the Maven wrapper version to allow checksum verification. Thanks.
Hi @showuon, can you create a custom GitHub Action on your fork that shows that the change works on a windows image (you can probably set up a matrix with all three OS to show that it's working for all of them).
@XComp , I've tried to run ./mvnw clean package -DskipTests in all 3 different OS via github action here, and so far the building process runs well. For the Windows env, it works well, too.
FYI, this is the workflow config file I created: https://github.com/showuon/flink/blob/master/.github/workflows/maven.yml , and this is the diff with the master branch: https://github.com/apache/flink/compare/master...showuon:flink:test_FLINK-33607?expand=1 . In the diff, it contains:
- The change in this PR
- The workflow config file (i.e.
.github/workflows/maven.yml) - skip the rat check (
apache-rat-plugin) to allow Windows passing license check.
Let me know if you have any other questions. Thanks.
Thanks for providing the workflow.
Isn't windows suppose to run with the mvn.cmd command? 🤔 I have no clue, I have never used Maven with Windows.
On the other note can you add a run without the change of this PR to simulate the issue with windows runs and w/o the Maven wrapper upgrade. Just to proof that it was actually failing beforehand and we're not testing something different. 🤔
Thanks for providing the workflow.
Isn't windows suppose to run with the
mvn.cmdcommand? 🤔 I have no clue, I have never used Maven with Windows.On the other note can you add a run without the change of this PR to simulate the issue with windows runs and w/o the Maven wrapper upgrade. Just to proof that it was actually failing beforehand and we're not testing something different. 🤔
@XComp , I can reproduce the MVRAPPER-103 described in using github action here. So, after I reverted the change in this PR, and add the wrapperSha256Sum line, it failed with:
Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised.
And this is the change I made for the branch: https://github.com/apache/flink/compare/master...showuon:flink:test_FLINK-33607_revert?expand=1 . You can see, in addition to some indent change after revert, I basically didn't add anything.
And to your question:
Isn't windows suppose to run with the
mvn.cmdcommand?
I confirmed in MWRAPPER-103 that the reporter is running with ./mvnw (see description in JIRA).
Let me know if you have any other questions. Thanks.
@XComp , I added another git action test today. I modified the wrapperSha256Sum to verify if we can successfully detect it, and the result works as expected here.
Run ./mvnw clean package -DskipTests
Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised.
Investigate or delete /home/runner/work/flink/flink/.mvn/wrapper/maven-wrapper.jar to attempt a clean download.
If you updated your Maven version, you need to update the specified wrapperSha256Sum property.
Error: Process completed with exit code 1.
Here's the change I made.
I think this PR is all verified. FYI.
@XComp , please help have another look when available. Thanks.
@XComp , kindly ping for another review. Thanks.
@XComp , kindly ping for another review. Thanks.
Sorry for the delay. Let's get this PR merged. Can you fix the PRs commit history:
- There shouldn't be any merge commit but only a single commit containing the actual change (just rebase the commit to most-recent version of master)
- The commit message needs to be updated to something like
[FLINK-33607][build] Updates Maven wrapper version and enables checksum verification for Maven wrapper
Sorry for the delay. Let's get this PR merged. Can you fix the PRs commit history:
* There shouldn't be any merge commit but only a single commit containing the actual change (just rebase the commit to most-recent version of master) * The commit message needs to be updated to something like `[FLINK-33607][build] Updates Maven wrapper version and enables checksum verification for Maven wrapper`
@XComp ,PR updated. Thanks!