Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-37941: [R][CI][Release] Add checksum verification for pre-compiled binaries #38115

Merged
merged 31 commits into from
Oct 11, 2023

Conversation

assignUser
Copy link
Member

@assignUser assignUser commented Oct 7, 2023

Rationale for this change

This change is to restore parity with the previous solution on macOS (brew does cs validation) and improve security for windows and linux. This also align with CRAN policy.

What changes are included in this PR?

This PR adds a script that can be run after the arrow release (once all files have been pushed to the artifactory) before the CRAN submission to download the checksum files for the pre-compiled binaries which are already added through the usual release. *libs.R have been extended to use these checksum files to validate the downloaded binaries.

Are these changes tested?

The r-binary-packages nightlies generate checksums and use them when building binary packages, this way the code path is tested. They do not modify the actual src package though.

Are there any user-facing changes?

no (outside of log messages)

@assignUser
Copy link
Member Author

assignUser commented Oct 7, 2023

@github-actions crossbow submit r-binary-packages

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@assignUser
Copy link
Member Author

assignUser commented Oct 7, 2023

@github-actions crossbow submit r-binary-packages

@github-actions

This comment was marked as outdated.

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Revision: 4dae43c

Submitted crossbow builds: ursacomputing/crossbow @ actions-88d6d2b34e

Task Status
conda-linux-aarch64-cpu-r42 Azure
conda-linux-aarch64-cpu-r43 Azure
conda-linux-x64-cpu-r42 Azure
conda-linux-x64-cpu-r43 Azure
conda-osx-arm64-cpu-r42 Azure
conda-osx-arm64-cpu-r43 Azure
conda-osx-x64-cpu-r42 Azure
conda-osx-x64-cpu-r43 Azure
conda-win-x64-cpu-r41 Azure
homebrew-r-autobrew Github Actions
homebrew-r-brew Github Actions
r-binary-packages Github Actions
test-fedora-r-clang-sanitizer Azure
test-r-arrow-backwards-compatibility Github Actions
test-r-depsource-bundled Azure
test-r-depsource-system Github Actions
test-r-dev-duckdb Github Actions
test-r-devdocs Github Actions
test-r-gcc-11 Github Actions
test-r-gcc-12 Github Actions
test-r-install-local Github Actions
test-r-install-local-minsizerel Github Actions
test-r-library-r-base-latest Azure
test-r-linux-as-cran Github Actions
test-r-linux-rchk Github Actions
test-r-linux-valgrind Azure
test-r-minimal-build Azure
test-r-offline-maximal Github Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-gcc-release-latest Azure
test-r-rstudio-r-base-4.1-opensuse153 Azure
test-r-rstudio-r-base-4.2-centos7-devtoolset-8 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 Github Actions
test-r-versions Github Actions
test-ubuntu-r-sanitizer Azure

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Two small comments...thank you for taking this on!

r/tools/nixlibs.R Outdated Show resolved Hide resolved
r/.gitignore Show resolved Hide resolved
@paleolimbot
Copy link
Member

Also, it is probably worth rebasing to clear up the CI.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 10, 2023
@assignUser
Copy link
Member Author

Also, it is probably worth rebasing to clear up the CI.

I merged to keep the crossbow job shas valid. I will run another round of validation but then this should be merge ready imo.

@assignUser
Copy link
Member Author

assignUser commented Oct 11, 2023

@github-actions crossbow submit -g r

@github-actions
Copy link

Revision: dd292b6

Submitted crossbow builds: ursacomputing/crossbow @ actions-d4c5399398

Task Status
conda-linux-aarch64-cpu-r42 Azure
conda-linux-aarch64-cpu-r43 Azure
conda-linux-x64-cpu-r42 Azure
conda-linux-x64-cpu-r43 Azure
conda-osx-arm64-cpu-r42 Azure
conda-osx-arm64-cpu-r43 Azure
conda-osx-x64-cpu-r42 Azure
conda-osx-x64-cpu-r43 Azure
conda-win-x64-cpu-r41 Azure
r-binary-packages Github Actions
test-fedora-r-clang-sanitizer Azure
test-r-arrow-backwards-compatibility Github Actions
test-r-depsource-bundled Azure
test-r-depsource-system Github Actions
test-r-dev-duckdb Github Actions
test-r-devdocs Github Actions
test-r-gcc-11 Github Actions
test-r-gcc-12 Github Actions
test-r-install-local Github Actions
test-r-install-local-minsizerel Github Actions
test-r-library-r-base-latest Azure
test-r-linux-as-cran Github Actions
test-r-linux-rchk Github Actions
test-r-linux-valgrind Azure
test-r-minimal-build Azure
test-r-offline-maximal Github Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-gcc-release-latest Azure
test-r-rstudio-r-base-4.1-opensuse153 Azure
test-r-rstudio-r-base-4.2-centos7-devtoolset-8 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 Github Actions
test-r-versions Github Actions
test-ubuntu-r-sanitizer Azure

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Pending green CI, naturally. Thank you!

r/tools/nixlibs.R Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 11, 2023
@thisisnic thisisnic merged commit b20e0ae into apache:main Oct 11, 2023
15 checks passed
@thisisnic thisisnic removed the awaiting changes Awaiting changes label Oct 11, 2023
Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

A few notes, apologies for the late review here @assignUser

r/tools/nixlibs.R Show resolved Hide resolved
checksum_cmd <- "shasum"
checksum_args <- c("--status", "-a", "512", "-c", checksum_file)

# shasum is not available on all linux versions
Copy link
Member

Choose a reason for hiding this comment

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

Use sys.which() to see if it's present?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nice 👍

r/tools/nixlibs.R Show resolved Hide resolved
r/tools/update-checksums.R Show resolved Hide resolved
r/tools/winlibs.R Show resolved Hide resolved
r/PACKAGING.md Show resolved Hide resolved
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Oct 11, 2023
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 11, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit b20e0ae.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…mpiled binaries (apache#38115)

### Rationale for this change

This change is to restore parity with the previous solution on macOS (brew does cs validation) and improve security for windows and linux. This also align with CRAN policy.

### What changes are included in this PR?

This PR adds a script that can be run after the arrow release (once all files have been pushed to the artifactory) before the CRAN submission to download the checksum files for the pre-compiled binaries which are already added through the usual release. *libs.R have been extended to use these checksum files to validate the downloaded binaries.

### Are these changes tested?

The r-binary-packages nightlies generate checksums and use them when building binary packages, this way the code path is tested. They do not modify the actual src package though.

### Are there any user-facing changes?
no (outside of log messages)
* Closes: apache#37941

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…mpiled binaries (apache#38115)

### Rationale for this change

This change is to restore parity with the previous solution on macOS (brew does cs validation) and improve security for windows and linux. This also align with CRAN policy.

### What changes are included in this PR?

This PR adds a script that can be run after the arrow release (once all files have been pushed to the artifactory) before the CRAN submission to download the checksum files for the pre-compiled binaries which are already added through the usual release. *libs.R have been extended to use these checksum files to validate the downloaded binaries.

### Are these changes tested?

The r-binary-packages nightlies generate checksums and use them when building binary packages, this way the code path is tested. They do not modify the actual src package though.

### Are there any user-facing changes?
no (outside of log messages)
* Closes: apache#37941

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…mpiled binaries (apache#38115)

### Rationale for this change

This change is to restore parity with the previous solution on macOS (brew does cs validation) and improve security for windows and linux. This also align with CRAN policy.

### What changes are included in this PR?

This PR adds a script that can be run after the arrow release (once all files have been pushed to the artifactory) before the CRAN submission to download the checksum files for the pre-compiled binaries which are already added through the usual release. *libs.R have been extended to use these checksum files to validate the downloaded binaries.

### Are these changes tested?

The r-binary-packages nightlies generate checksums and use them when building binary packages, this way the code path is tested. They do not modify the actual src package though.

### Are there any user-facing changes?
no (outside of log messages)
* Closes: apache#37941

Authored-by: Jacob Wujciak-Jens <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R][Release] Add checksum verification to pre-compiled binaries
4 participants