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

Relax zstd-sys Version Pin #5829

Merged
merged 7 commits into from
Jun 1, 2024
Merged

Relax zstd-sys Version Pin #5829

merged 7 commits into from
Jun 1, 2024

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented Jun 1, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

I hit this while trying to upgrade arrow for datafusion.

Parquet depends on zstd-sys and datafusion depends on async-compression which then depends on zstd-sys via zstd-safe.

Parquet cannot use zstd-sys beyond 2.0.9 due to gyscos/zstd-rs#269. And zstd-safe has two available versions 7.0.0 which depends on zstd-sys 2.0.7 and 7.1.0 on zstd-sys 2.10.0.

So to make parquet be compatible with projects use async-compression, the only available version to zstd-sys is 2.0.7

What changes are included in this PR?

Downgrade and pin zstd-sys to 2.0.7

Are there any user-facing changes?

Yes, the dependences

This patch needs to be included in the next release #5688 if possible

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@github-actions github-actions bot added the parquet Changes to the parquet crate label Jun 1, 2024
@kylebarron
Copy link
Contributor

Maybe <= 2.0.9?

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia
Copy link
Member Author

waynexia commented Jun 1, 2024

Maybe <= 2.0.9?

That looks better 👍 Changed in fc4c06a

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@@ -55,7 +55,7 @@ lz4_flex = { version = "0.11", default-features = false, features = ["std", "fra
zstd = { version = "0.13.0", optional = true, default-features = false }
# TODO: temporary to fix parquet wasm build
# upstream issue: https://github.com/gyscos/zstd-rs/issues/269
zstd-sys = { version = "=2.0.7", optional = true, default-features = false }
zstd-sys = { version = "<=2.0.9", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

This could pick up zstd-sys 1.0, please make sure we also have >= 2.0.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, that works 💯

# TODO: temporary to fix parquet wasm build
# upstream issue: https://github.com/gyscos/zstd-rs/issues/269
zstd = { version = "<0.13.1", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

The same, <0.13.1 here will also pick 0.12, 0.11.

@waynexia

This comment was marked as outdated.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@tustvold
Copy link
Contributor

tustvold commented Jun 1, 2024

I'm somewhat inclined to think we should just remove the version pin, and document that people using wasm32 will need to manually pin zstd-sys themselves. We're creating pain for all of our users by pinning this crate, to save some pain for a very small subset of our community... Ultimately if zstd-sys broke support for wasm32 it is on them to fix it

@Xuanwo
Copy link
Member

Xuanwo commented Jun 1, 2024

I'm somewhat inclined to think we should just remove the version pin, and document that people using wasm32 will need to manually pin zstd-sys themselves.

I agree that libs like parquet should not pin dependencies. This forces users to adopt the same pinned version, increasing the likelihood of conflicts. Instead of pinning versions downstream, it's better for us to collaborate upstream to address the issue.

@waynexia
Copy link
Member Author

waynexia commented Jun 1, 2024

I'm somewhat inclined to think we should just remove the version pin, and document that people using wasm32 will need to manually pin zstd-sys themselves. We're creating pain for all of our users by pinning this crate, to save some pain for a very small subset of our community... Ultimately if zstd-sys broke support for wasm32 it is on them to fix it

That's also an option. Cargo provides patch mechanism to override deps

@tustvold
Copy link
Contributor

tustvold commented Jun 1, 2024

Cargo provides patch mechanism to override deps

They shouldn't need to go that far, if they have a lockfile they can just use cargo update --precise, otherwise they can just declare an explicit dependency on zstd-sys and pin it there.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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.

You will need to update the wasm CI to pin zstd-sys

# upstream issue: https://github.com/gyscos/zstd-rs/issues/269
zstd-sys = { version = "=2.0.9", optional = true, default-features = false }
zstd = { version = "0.13", optional = true, default-features = false }
zstd-sys = { version = "2.0", 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.

I suspect zstd-sys can be removed entirely

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to target-specified deps (for CI)

@waynexia
Copy link
Member Author

waynexia commented Jun 1, 2024

Removed them in d7d5e9b based on above discussion

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@tustvold tustvold changed the title build: pin zstd-sys to 2.0.7 Relax zstd-sys Version Pin Jun 1, 2024
@tustvold tustvold merged commit e4b28bd into apache:master Jun 1, 2024
18 checks passed
@waynexia waynexia deleted the pin-zstd-sys branch June 1, 2024 14:06
@waynexia
Copy link
Member Author

waynexia commented Jun 1, 2024

Thank you all ❤️

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.

4 participants