Backport fix for zlib inflation with custom dicts to 4.x#9358
Backport fix for zlib inflation with custom dicts to 4.x#9358thusoy wants to merge 1 commit intonodejs:v4.x-stagingfrom
Conversation
|
Uh, there are a few too many commits here – could you (Also, sorry, it’s totally likely this is not your fault. Sometimes there’s no other way than force-pushing to the staging branches and making people rebase their backport PRs.) |
f9f1283 to
779f9e1
Compare
|
Sorry, just noticed, hadn't updated my local v4.x-staging branch. Fixed now. |
|
Note that I included the |
779f9e1 to
ea90daa
Compare
There was a problem hiding this comment.
The inflate.setEncoding('utf-8'); call seems to be missing?
There was a problem hiding this comment.
I think you can use assert.strictEqual in v4.x, too
There was a problem hiding this comment.
So just to be clear, do you want me to backport both of the commits from last time, both the actual fix and the test fixes? I thought you said just to backport the first, that's why I didn't include the test refactors, but I might have misunderstood. I'd be happy to do them both if that's what you want.
There was a problem hiding this comment.
@thusoy That’s basically your call, and you don’t need to fix these. I would expect backporting the second commit to be more prone to merge conflicts and just generally a bit more work, but of course it’s appreciated if that’s something you’d like to do (partially or wholly).
I was just commenting here because the diff looks different from the one in #8512, that’s all.
There was a problem hiding this comment.
Seeing how different the tests are here I imagine backporting the other one would be quite painful, yes, I don't think I'm masochistic enough for that. I can address the points you raise if you like, but they were not part of the original first commit either, so I skipped them to keep the same style as the surrounding code.
There was a problem hiding this comment.
Exactly. :) I’m fine then as long as the tests are passing.
(edit: I’d kick off CI if it wasn’t timing out again…)
There was a problem hiding this comment.
Cool, at your leisure then. (For the record, it Works On My Machine (tm)).
There was a problem hiding this comment.
(ditto for inflate.setEncoding('utf-8');)
|
Is that FreeBSD failure related to this? I couldn't figure out how to find the build failure output. |
|
Huh – I don’t think so. The compiler output is at https://ci.nodejs.org/job/node-test-commit-freebsd/5120/nodes=freebsd11-x64/console … maybe someone from @nodejs/build knows what’s up with that? Should the Freebsd 11 job maybe be skipped for v4? |
15ec16e to
9049c1f
Compare
|
@thusoy this needs to be rebased. I tried cherry-picking the single commit but was getting failures in |
Moves inflateSetDictionary right after inflateInit2 when mode is INFLATERAW, since without the wrapper in appears zlib won't return Z_NEED_DICT as it would otherwise, and will thus attempt inflating without the dictionary, leading to an error.
ea90daa to
96611b8
Compare
|
Okay so I’ve taken the liberty to use the “Allow edits from maintainers” feature and rebased this, I think that’s okay here. New CI: https://ci.nodejs.org/job/node-test-commit/6091/
@thealphanerd I didn’t/don’t… Can you still reproduce that? (Current |
|
landed in 9a02414 |
|
Great, thanks to both of you! On November 21, 2016 8:58:47 PM PST, Myles Borins notifications@github.com wrote:
Have a splendid day! Sent from phone, thus potentially short. Tarjei |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
zlib
Description of change
Backport of #8512 as requested in comments.