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 zlib-ng library to 2.2.2 #219

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

EliahKagan
Copy link
Contributor

This updates the zlib-ng submodule from the 2.2.1 tag to the 2.2.2 tag, since the patch release 2.2.2 came out several days ago.

I had originally looked into this in the hope that it would help with #200, but as described in #200 (comment) it looks like it does not help with that at least on some systems including the RISC-V test system I am using, even though it includes a change that tries to do so. Nonetheless, it seems to me that it may be valuable to bump the submodule to this new stable bugfix/patch release, in view of its several improvements.

The submodule version was last increased here in 33dff11 (#211). Unlike that change, this one is just a patch release, and as far as I can tell from building it locally, it looks like just bumping the submodule commit ID, without having to modify any build scripts or other files, may be sufficient here.

If this PR is merged, that does not necessarily require an immediate new release of this project's crates. I'm not sure if it's best to do that as well now or to wait.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking us in the right direction.

One can hope that after the interaction with the upstream zlib-ng project, the RISCV issue can be fully resolved.

As it still reproducibly crashes when trying it with gix, and there seem to be no substantial or requested changes in zlib-ng, it seems fair not to create a new release of this crate at this time.

@Byron Byron merged commit cbfee75 into rust-lang:main Sep 22, 2024
48 checks passed
@EliahKagan EliahKagan deleted the update-zlib-ng-2-2-2 branch September 22, 2024 07:22
@EliahKagan
Copy link
Contributor Author

EliahKagan commented Sep 22, 2024

One can hope that after the interaction with the upstream zlib-ng project, the RISCV issue can be fully resolved.

Yes. I wanted to test with the upstream test suite before posting there, but I have done so and posted zlib-ng/zlib-ng#1705 (comment).

As it still reproducibly crashes when trying it with gix, and there seem to be no substantial or requested changes in zlib-ng, it seems fair not to create a new release of this crate at this time.

Yes, actually there are more upstream test failures on my test system with 2.2.2 than 2.2.1 (all failures with both versions are due to RVV being used and go away when -DWITH_RVV=OFF is passed when configuring with cmake). I was unaware of that at the time I opened this pull request, which is why I didn't mention it here.

I am unsure if this really translates into more practical situations where a problem would arise. My guess is that such scenarios are uncommon, and thus that 2.2.2 is still overall better than 2.2.1 (mainly due to having zlib-ng/zlib-ng#1773). So if people report they need the other fixes present in 2.2.2, then I think it could be okay to do a release here, even without further progress on RVV detection. Otherwise I very much agree it's better to put it off.

@EliahKagan
Copy link
Contributor Author

Since the goal of #218 remains relevant in 2.2.2, I've rebased that and adjusted it in a way that moves it as close as I think it can reasonably go, at this time, to what has been suggested. In some cases, it was necessary to choose between doing something more complicated than had been discussed, or something simpler. I resolved those cases by doing the simpler thing.

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