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

Workaround weird regression with &[u8] and zopfli #595

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrews05
Copy link
Collaborator

Fixes #579. In lack of a proper understanding of what's going on here and why the issue is happening, this workaround will do for now.

@AlexTMjugador
Copy link
Collaborator

I'd like to fix this in the Zopfli crate itself instead, but let's keep this PR open in case I'm unable to do so in a timely fashion.

@andrews05
Copy link
Collaborator Author

Sounds good. I'm thinking we should do a point release once this is fixed.

@XhmikosR
Copy link
Contributor

XhmikosR commented Mar 8, 2024

@andrews05 could you please rebase the branch so that I can grab the latest CI build for testing? Thanks!

@andrews05
Copy link
Collaborator Author

@XhmikosR Done

@ace-dent
Copy link

Is the artefact build good enough for some regression testing? Or should I wait for the point release?

@andrews05
Copy link
Collaborator Author

andrews05 commented Mar 11, 2024

Yeah, it should be perfectly good for testing, feel free. I’ve just rebased again so you should see a new build in a moment.

Note you may see some cases where the “fix” makes it worse, but on average it’s an improvement in my observations.

@andrews05
Copy link
Collaborator Author

Oh, looks like there are some new clippy lints I need to fix. I’ll try and sort that soon…

@andrews05
Copy link
Collaborator Author

andrews05 commented Mar 11, 2024

Success!

@AlexTMjugador What do you think, will you have time to investigate or shall we just use this for now?

@XhmikosR
Copy link
Contributor

@andrews05 just my 2 cents. The clippy fixes could go into master separately, but this makes me wonder how come you guys are hitting this inconsistency. I believe you need to enable the repo option for branches to always be up to date so that you catch such issues before landing something in master.

@AlexTMjugador
Copy link
Collaborator

I think the Clippy lint fixes should have been pushed to the main branch separately, so that this and other PRs could be rebased on top of those changes and benefit them all. But I'm doing the cherry-picking and rebasing now, so no worries here.

What do you think, will you have time to investigate or shall we just use this for now?

Next week I finally have some days off which I could use to dedicate some time to open source work like this, but no promises 🙂

@ace-dent
Copy link

Hey gang... any chance of rebasing to main, to regenerate the CI artefacts? 🙏🏻

@andrews05
Copy link
Collaborator Author

@ace-dent done. Let me know your observations with and without this patch.

@ace-dent
Copy link

@andrews05 - nice! Thanks!

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.

Probably regression in v9.0.0 compared to v8.0.0, compression became worse
4 participants