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

Pin zstd-sys to v2.0.9 in parquet #5567

Merged
merged 5 commits into from
Mar 29, 2024

Conversation

Jefffrey
Copy link
Contributor

@Jefffrey Jefffrey commented Mar 29, 2024

Which issue does this PR close?

Closes #5565

Rationale for this change

Using 2.0.10 causes parquet wasm build to fail. Can pin to 2.0.9 to have succeeding CI again, and maybe wait for upstream to fix issue with higher versions.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Mar 29, 2024
@@ -53,6 +53,7 @@ brotli = { version = "3.3", default-features = false, features = ["std"], option
flate2 = { version = "1.0", default-features = false, features = ["rust_backend"], optional = true }
lz4_flex = { version = "0.11", default-features = false, features = ["std", "frame"], optional = true }
zstd = { version = "0.13.0", optional = true, default-features = false }
zstd-sys = { version = "=2.0.9", optional = true, default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should make a new feature zstd that turns on both the zstd and the zstd-sys features? Should this be a range for >=2.0,<2.10?

Copy link
Contributor Author

@Jefffrey Jefffrey Mar 29, 2024

Choose a reason for hiding this comment

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

Yeah that's a good point 👍

I was just doing some testing here so I'll clean it up as suggested

Edit: although, is it worth introducing a new feature for what will hopefully be a temporary fix? 🤔

Edit2: nvm, just needed to play with features a bit

@Jefffrey Jefffrey changed the title Test pinning zstd-sys Pin zstd-sys to v2.0.9 in parquet Mar 29, 2024
@Jefffrey Jefffrey marked this pull request as ready for review March 29, 2024 03:25
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Is there an upstream ticket for this we can link to?

@Jefffrey
Copy link
Contributor Author

Is there an upstream ticket for this we can link to?

Updated in the code 👍

@tustvold tustvold merged commit 3c50d47 into apache:master Mar 29, 2024
17 checks passed
kylebarron added a commit to geoarrow/geoarrow-rs that referenced this pull request Apr 4, 2024
@Jefffrey Jefffrey deleted the test_parquet_wasm_ci branch April 22, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parquet / Build wasm32 (pull_request) CI check failing on main
3 participants