Skip to content

Backport fix for zlib inflation with custom dicts to 4.x - #9358

Closed
thusoy wants to merge 1 commit into
nodejs:v4.x-stagingfrom
thusoy:backport-issue-8507
Closed

Backport fix for zlib inflation with custom dicts to 4.x#9358
thusoy wants to merge 1 commit into
nodejs:v4.x-stagingfrom
thusoy:backport-issue-8507

Conversation

@thusoy

@thusoythusoy commented Oct 29, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

zlib

Description of change

Backport of #8512 as requested in comments.

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. openssl Issues and PRs related to the OpenSSL dependency. stream Issues and PRs related to the stream subsystem. v4.x labels Oct 29, 2016
@addaleax

addaleax commented Oct 29, 2016

Copy link
Copy Markdown
Member

Uh, there are a few too many commits here – could you git rebase -i upstream/v4.x-staging and make sure only the one you actually want is there? :)

(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.)

@thusoy
thusoyforce-pushed the backport-issue-8507 branch from f9f1283 to 779f9e1CompareOctober 29, 2016 20:29
@thusoy

thusoy commented Oct 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Sorry, just noticed, hadn't updated my local v4.x-staging branch. Fixed now.

@thusoy

thusoy commented Oct 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Note that I included the common.mustCall() in test/parallel/test-zlib-dictionary.js this one to ensure that the test added are run, even though that was originally added in the test fix commit that followed, but I didn't include any of the other fixes.

@thusoy
thusoyforce-pushed the backport-issue-8507 branch from 779f9e1 to ea90daaCompareOctober 29, 2016 20:36
addaleax
addaleax reviewed Oct 29, 2016
View reviewed changes
Comment threadtest/parallel/test-zlib-dictionary.js Outdated

@addaleaxaddaleaxOct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The inflate.setEncoding('utf-8'); call seems to be missing?

Comment threadtest/parallel/test-zlib-dictionary.js Outdated

@addaleaxaddaleaxOct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you can use assert.strictEqual in v4.x, too

@thusoythusoyOct 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@addaleaxaddaleaxOct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

@thusoythusoyOct 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@addaleaxaddaleaxOct 29, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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…)

@thusoythusoyOct 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Cool, at your leisure then. (For the record, it Works On My Machine (tm)).

Comment threadtest/parallel/test-zlib-dictionary.js Outdated

@addaleaxaddaleaxOct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(ditto for inflate.setEncoding('utf-8');)

@addaleaxaddaleax added zlib Issues and PRs related to the zlib subsystem. and removed build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. openssl Issues and PRs related to the OpenSSL dependency. stream Issues and PRs related to the stream subsystem. labels Oct 29, 2016
@addaleax

addaleax commented Oct 30, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/5870/

@thusoy

thusoy commented Oct 30, 2016

Copy link
Copy Markdown
ContributorAuthor

Is that FreeBSD failure related to this? I couldn't figure out how to find the build failure output.

@addaleax

addaleax commented Oct 30, 2016

Copy link
Copy Markdown
Member

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?

@MylesBorins

MylesBorins commented Nov 14, 2016

Copy link
Copy Markdown
Contributor

@thusoy this needs to be rebased. I tried cherry-picking the single commit but was getting failures in parallel/test-tls-server-verify

@MylesBorinsMylesBorins added this to the 4.7.0 milestone Nov 15, 2016
@thusoy@addaleax
zlib: fix raw inflate with custom dictionary
96611b8
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.
@addaleax

addaleax commented Nov 20, 2016

Copy link
Copy Markdown
Member

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/

I tried cherry-picking the single commit but was getting failures in parallel/test-tls-server-verify

@thealphanerd I didn’t/don’t… Can you still reproduce that? (Current HEAD of this PR was @ ea90daa when you wrote that)

@MylesBorins

MylesBorins commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

landed in 9a02414

@thusoy

thusoy commented Nov 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Great, thanks to both of you!

On November 21, 2016 8:58:47 PM PST, Myles Borins notifications@github.com wrote:

Closed #9358.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#9358 (comment)

Have a splendid day!

Sent from phone, thus potentially short.

Tarjei

@MylesBorinsMylesBorins removed this from the 4.7.0 milestone Nov 22, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

zlibIssues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thusoy@addaleax@MylesBorins@nodejs-github-bot