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

Update Dependencies and Refactor Code for zip Crate Breaking Changes #245

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

mxsrm
Copy link
Contributor

@mxsrm mxsrm commented Dec 1, 2024

Pull Request: Update Dependencies and Refactor Code for zip Crate Breaking Changes

This pull request updates the project's dependencies to their latest versions, addressing breaking changes introduced by the zip crate. Additionally, the codebase has been refactored to accommodate these changes and align with current Rust practices.

Key Changes

  1. Dependency Updates

    • hashbrown: 0.14.50.15.2
    • quick-xml: 0.36.20.37.1
    • zip: 1.1.42.2.1
  2. Refactoring

    • The WriterManager struct and related methods have been updated to work with a mutable reference to ZipWriter instead of consuming it directly. This improves flexibility and reduces unnecessary ownership overhead.
    • Adjusted Cells logic to replace insert_unique_unchecked with HashMap::insert, improving safety and aligning with standard practices.
  3. Added resolver = "2"

    • The new dependency resolver was enabled in Cargo.toml to resolve compatibility issues caused by dependency version conflicts.

Known Issue: wasm-bindgen Bug Prevents Build

Currently, the code does not build due to a critical bug introduced in the wasm-bindgen crate. The issue stems from a dependency chain as shown below:

wasm-bindgen-macro v0.2.97 (proc-macro)
└── wasm-bindgen v0.2.97
    ├── iana-time-zone v0.1.61
    │   └── chrono v0.4.38
    │       └── umya-spreadsheet v2.1.2
    ├── js-sys v0.3.74
    │   └── iana-time-zone v0.1.61 (*)
    ├── rav1e v0.7.1
    │   └── ravif v0.11.11
    │       └── image v0.25.5
    │           └── umya-spreadsheet v2.1.2
    └── v_frame v0.3.8
        ├── av1-grain v0.2.3
        │   └── rav1e v0.7.1 (*)
        └── rav1e v0.7.1 (*)

To fix this issue manually, the following change is required:

diff --git a/Cargo.toml b/Cargo.toml
index db40a10..ff7756f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,6 +7,7 @@ keywords = ["excel", "spreadsheet", "xlsx", "reader", "writer"]
 license = "MIT"
 readme = "README.md"
 description = "umya-spreadsheet is a library written in pure Rust to read and write xlsx file."
+resolver = "2"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

Additional Notes

This wasm-bindgen bug also affects the main branch as it stands, so enabling resolver = "2" is necessary for both the main branch and this PR.

Once the upstream issue in wasm-bindgen is resolved, this branch should build correctly without further intervention.

mxsrm added 2 commits December 1, 2024 14:57
- Upgraded dependencies:
  - `hashbrown` from 0.14.5 to 0.15.2
  - `quick-xml` from 0.36.2 to 0.37.1
  - `zip` from 1.1.4 to 2.2.1

Cells: replace `insert_unique_unchecked` with `insert`

- Updated the insertion logic in `Cells` to use `HashMap::insert` instead of the custom `insert_unique_unchecked`. This change ensures better safety and aligns with standard practices.

WriterManager: refactor to use mutable reference for `ZipWriter`

- Modified `WriterManager` to hold a mutable reference (`&mut`) to `ZipWriter` instead of consuming it directly.
- Adjusted the constructor and methods accordingly to reflect this change, improving flexibility and reducing unnecessary ownership transfers.
- This change was necessary due to breaking changes in the `zip` crate

Writer XLSX: fix `make_buffer` logic

- Refactored `make_buffer` to create and pass a mutable reference of `ZipWriter` to `WriterManager`.
- Simplified the logic for finalizing the archive by directly calling `arv.finish()` and returning the result.

These changes improve code safety, flexibility, and maintain compatibility with updated dependencies.
@mxsrm mxsrm changed the title Update dependencies Update Dependencies and Refactor Code for zip Crate Breaking Changes Dec 1, 2024
@mxsrm
Copy link
Contributor Author

mxsrm commented Dec 1, 2024

The internal refactoring was necessary because the finish() function in the zip crate was updated to consume the ZipWriter, as introduced in this commit. To accommodate this change, WriterManager now holds a mutable reference to the ZipWriter instead of consuming it directly.

The insert_unique_unchecked() function from hashbrown has been made unsafe, thus I had to change that call to the safe insert() version.

The `hashbrown` crate was unnecessary because `std::collections::HashMap` is already the default implementation in the standard library, based on `hashbrown`. This change removes the `hashbrown` dependency and updates all imports to use `std::collections::HashMap` instead, reducing the dependency surface and aligning with standard conventions.
@mxsrm
Copy link
Contributor Author

mxsrm commented Dec 1, 2024

The additional commit db119108d53124c92fbca69963d11bfc2746d979 removes the hashbrown dependency completely.

Reason:

Since Rust 1.36, this is now the HashMap implementation for the Rust standard library. However you may still want to use this crate instead since it works in environments without std, such as embedded systems and kernels.

Source: https://github.com/rust-lang/hashbrown

@MathNya
Copy link
Owner

MathNya commented Dec 2, 2024

@mxsrm
Thank you for the PR.
We have no problem with the fixes and will merge them.
We also found out about the problem with wasm-bindgen.
If it has not been fixed by the time the new version of umya-spreadsheet is released, we will address it with resolver = “2”.

@MathNya MathNya merged commit da7dc5e into MathNya:master Dec 2, 2024
1 check failed
@mxsrm mxsrm deleted the update-deps branch December 21, 2024 18:25
Sign up for free to 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.

2 participants