Skip to content

[rust] Use apple-flat-package crate to extract PKG files#13740

Merged
diemol merged 7 commits intotrunkfrom
sm_extract_pkg
Mar 26, 2024
Merged

[rust] Use apple-flat-package crate to extract PKG files#13740
diemol merged 7 commits intotrunkfrom
sm_extract_pkg

Conversation

@bonigarcia
Copy link
Copy Markdown
Member

@bonigarcia bonigarcia commented Mar 26, 2024 •
edited by qodo-code-review bot
Loading

User description

Description

This PR uses an existing crate called apple-flat-package to uncompress PKG artifacts, required to manage Firefox and Edge in macOS.

Motivation and Context

Previously, SM used the shell command pkgutil to this aim. Nevertheless, this extraction is not working properly in GHA (although in local machines seems to continue working).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

bug_fix, enhancement

Description

  • Replaced the use of shell commands with the apple_flat_package crate for uncompressing PKG files, enhancing reliability and portability.
  • Removed obsolete code and simplified function calls following the new PKG uncompression approach.
  • Added apple-flat-package to dependencies for handling PKG file extraction.

Changes walkthrough

Relevant files
Enhancement
files.rs
Replace Shell Command with Crate for PKG Uncompression     

rust/src/files.rs

  • Replaced shell command-based PKG uncompression with apple_flat_package
    crate usage.
  • Removed unused major_browser_version parameter from uncompress_pkg
    function.
  • Added logic to extract PKG files using PkgReader from
    apple_flat_package.
  • Implemented file extraction and permissions setting for Unix systems.
    		•  +33/-48 
    lib.rs
    Cleanup and Simplification after PKG Uncompression Update

    rust/src/lib.rs

  • Removed unused constants related to the old PKG uncompression method.
  • Simplified uncompress function calls by removing the
    major_browser_version parameter.
    		•  +0/-9     
    Dependencies
    Cargo.toml
    Add Dependency for PKG File Handling                                         

    rust/Cargo.toml

    • Added apple-flat-package crate to dependencies.
    		 +1/-0     

    ✨ PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @bonigarcia bonigarcia added the C-rust Rust code is mostly Selenium Manager label Mar 26, 2024
    @qodo-code-review qodo-code-review bot added P-enhancement PR with a new feature bug_fix labels Mar 26, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 26, 2024

    PR Description updated to latest commit (683967b)

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 26, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves integrating a new crate for handling PKG files, replacing shell command logic, and removing obsolete code. The changes are moderate in size and complexity, requiring a reviewer to understand Rust, file handling, and the specific requirements of handling PKG files in macOS.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Error Handling: The new uncompress_pkg function could improve its error handling. For example, it assumes the first package in the PKG file is always the correct one to extract without handling cases where this might not be true.

    File Permissions: The code sets file permissions explicitly on Unix systems. This is generally fine, but it should ensure that it doesn't inadvertently set insecure permissions on sensitive files.

    Hardcoded Expectation: The function uncompress_pkg expects at least one package in the PKG file (packages.first().ok_or(anyhow!("Unable to extract PKG"))?;). While this might be a reasonable assumption, it could be more robust against malformed or unexpected PKG files.

    🔒 Security concerns

    No specific security concerns are introduced in the PR code. However, it's important to ensure that the apple-flat-package crate is securely handling file extraction to avoid vulnerabilities such as path traversal attacks.
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 26, 2024 •
    edited
    Loading

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Security
    Add path sanitization to prevent directory traversal vulnerabilities.

    Consider handling the case where entry.name() returns a path that traverses outside the
    intended directory (e.g., via ../ sequences). This can help prevent potential directory
    traversal vulnerabilities. You can use canonicalization or path sanitization techniques to
    ensure that extracted files do not end up outside the target directory.

    rust/src/files.rs [187-190]

     let name = entry.name();
+let sanitized_name = sanitize_path(name)?;
 let mut file = Vec::new();
 cpio_reader.read_to_end(&mut file)?;
-let target_path_buf = target_path.join(name);
+let target_path_buf = target_path.join(sanitized_name);
    Verify the integrity and authenticity of the PKG file before extraction.

    Consider verifying the integrity and authenticity of the PKG file before extracting it.
    This can be done by checking digital signatures or checksums. This step is crucial for
    security, especially if the PKG file comes from an untrusted source.

    rust/src/files.rs [181]

    +// Example: Verify the PKG file's signature or checksum before proceeding
+verify_pkg_integrity(compressed_file)?;
 let mut reader = PkgReader::new(File::open(compressed_file)?)?;
    Enhancement
    Use a library or dedicated function for CPIO extraction.

    Instead of manually iterating over the cpio_reader and writing files, consider using a
    more robust library or existing functionality that handles the nuances of CPIO extraction,
    including directory creation, file metadata, and error handling in a more comprehensive
    manner.

    rust/src/files.rs [184-206]

     if let Some(mut cpio_reader) = package.payload_reader()? {
-    while let Some(next) = cpio_reader.next() {
-        let entry = next?;
-        ...
-    }
+    extract_cpio(cpio_reader, target_path)?;
 }
    Enhance error messages for better debugging and user feedback.

    To improve error handling, consider providing more context in the error message when
    unable to extract the PKG. This could include the path of the PKG file being processed.
    Using anyhow! with a more descriptive error message can aid in debugging and user
    feedback.

    rust/src/files.rs [183]

    -let package = packages.first().ok_or(anyhow!("Unable to extract PKG"))?;
+let package = packages.first().ok_or(anyhow!("Unable to extract PKG: {}", compressed_file))?;
    Best practice
    Add a check for None before unwrapping target_path.parent().

    It's a good practice to check if the target_path.parent() is None before unwrapping it to
    avoid potential panics. Consider handling the case where parent() might return None, which
    can happen if target_path is a root directory.

    rust/src/files.rs [194]

    -fs::create_dir_all(target_path.parent().unwrap())?;
+if let Some(parent_path) = target_path.parent() {
+    fs::create_dir_all(parent_path)?;
+} else {
+    // Handle the case where the target path has no parent (e.g., root directory)
+}
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 26, 2024 •
    edited
    Loading

    CI Failure Feedback

    (Checks updated until commit 6e00341)

    Action: Rust / Tests (ubuntu) / Tests (ubuntu)

    Failed stage: Run Bazel [❌]

    Failed test name: //rust:unit

    Failure summary:

    The action failed due to a missing edition attribute in the rust_test rule for the target
    //rust:unit. This attribute is required for the test to run, as indicated by the error message
    "Attribute edition is required for @//rust:unit." This issue prevented the analysis of the target
    //rust:unit, leading to the build being aborted and no test targets being found.

    Relevant error logs: 
    1:  ##[group]Operating System
2:  Ubuntu
...

963:  Package 'php-symfony-debug-bundle' is not installed, so not removed
964:  Package 'php-symfony-dependency-injection' is not installed, so not removed
965:  Package 'php-symfony-deprecation-contracts' is not installed, so not removed
966:  Package 'php-symfony-discord-notifier' is not installed, so not removed
967:  Package 'php-symfony-doctrine-bridge' is not installed, so not removed
968:  Package 'php-symfony-doctrine-messenger' is not installed, so not removed
969:  Package 'php-symfony-dom-crawler' is not installed, so not removed
970:  Package 'php-symfony-dotenv' is not installed, so not removed
971:  Package 'php-symfony-error-handler' is not installed, so not removed
...

1846:  �[32mAnalyzing:�[0m 37 targets (173 packages loaded, 1646 targets configured)
1847:  �[32m[1 / 1]�[0m checking cached actions
1848:  �[32mAnalyzing:�[0m 37 targets (255 packages loaded, 6644 targets configured)
1849:  �[32m[1 / 1]�[0m checking cached actions
1850:  �[32mAnalyzing:�[0m 37 targets (372 packages loaded, 10751 targets configured)
1851:  �[32m[1 / 1]�[0m checking cached actions
1852:  �[32mAnalyzing:�[0m 37 targets (383 packages loaded, 11029 targets configured)
1853:  �[32m[1 / 1]�[0m checking cached actions
1854:  �[31m�[1mERROR: �[0m/home/runner/work/selenium/selenium/rust/BUILD.bazel:98:10: in rust_test rule //rust:unit: 
1855:  Traceback (most recent call last):
1856:  File "/home/runner/.bazel/external/rules_rust/rust/private/rust.bzl", line 399, column 34, in _rust_test_impl
1857:  edition = get_edition(ctx.attr, toolchain, ctx.label),
1858:  File "/home/runner/.bazel/external/rules_rust/rust/private/utils.bzl", line 824, column 13, in get_edition
1859:  fail("Attribute `edition` is required for {}.".format(label))
1860:  Error in fail: Attribute `edition` is required for @//rust:unit.
1861:  �[31m�[1mERROR: �[0m/home/runner/work/selenium/selenium/rust/BUILD.bazel:98:10: Analysis of target '//rust:unit' failed
1862:  �[32mAnalyzing:�[0m 37 targets (383 packages loaded, 11037 targets configured)
1863:  �[32m[1 / 1]�[0m checking cached actions
1864:  Target //rust:enable-rustfmt up-to-date (nothing to build)
1865:  �[31m�[1mERROR: �[0mAnalysis of target '//rust:unit' failed; build aborted
1866:  �[32mINFO: �[0mElapsed time: 20.689s, Critical Path: 1.50s
1867:  �[32mINFO: �[0m1 process: 1 internal.
1868:  �[31m�[1mERROR: �[0mBuild did NOT complete successfully
1869:  �[31m�[1mFAILED:�[0m 
1870:  �[31m�[1mERROR: �[0mNo test targets were found, yet testing was requested
1871:  �[0m
1872:  ##[error]Process completed with exit code 1.
    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @diemol
    Copy link
    Copy Markdown
    Member

    diemol commented Mar 26, 2024

    Does the SM binary grow?

    @bonigarcia
    Copy link
    Copy Markdown
    Member Author

    bonigarcia commented Mar 26, 2024

    This PR fixes the Rust build. In addition to uses the previously described crate to extract PKG files, this PR bumps the version of the rules_rust. We were using a very old version, and that was causing several issues. Now everything seems to work nicely.

    Does the SM binary grow?

    Not really. You can see the size of the resulting binaries at the bottom of the workflow run:

    https://github.com/SeleniumHQ/selenium/actions/runs/8442841426

    I don't see differences with respect to the current stable binaries:

    https://github.com/SeleniumHQ/selenium/blob/trunk/common/selenium_manager.bzl

    @diemol
    Copy link
    Copy Markdown
    Member

    diemol commented Mar 26, 2024

    Nice, I will merge after CI passes again.

    diemol
    diemol approved these changes Mar 26, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @bonigarcia!

    @diemol diemol merged commit 0c81991 into trunk Mar 26, 2024
    @diemol diemol deleted the sm_extract_pkg branch March 26, 2024 22:32
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    C-rust Rust code is mostly Selenium Manager P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    Selenium Manager
    Status: Done

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @bonigarcia @diemol