test: check zlib version for createDeflateRaw by danbev · Pull Request #13697 · nodejs/node

danbev · 2017-06-15T12:00:05Z

We are currenly builing Node with --shared-zlib which happens to be
version 1.2.8. The test for zlib.createDeflateRaw is expected to fail
but does not when using version 1.2.8.

As far as I can tell the fix referred to in the comments was
introduced in version 1.2.9:

- Reject a window size of 256 bytes if not using the zlib wrapper

This commit suggests adding a check for the version and skipping this
assert if the version is less than 1.2.9.

Refs: http://zlib.net/ChangeLog.txt

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

We are currenly builing Node with --shared-zlib which happens to be version 1.2.8. The test for zlib.createDeflateRaw is expected to fail but does not when using version 1.2.8. As far as I can tell the fix referred to in the comments was introduced in version 1.2.9: - Reject a window size of 256 bytes if not using the zlib wrapper This commit suggests adding a check for the version and skipping this assert if the version is less than 1.2.9. Refs: http://zlib.net/ChangeLog.txt

danbev · 2017-06-15T12:00:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8673/

danbev · 2017-06-15T13:20:06Z

test/osx failure looks unrelated

console output:

not ok 168 async-hooks/test-callback-error --- duration_ms: 60.87 severity: fail stack: |- timeout

benjamingr · 2017-06-15T14:12:28Z

-assert.throws(() => {
- zlib.createDeflateRaw({ windowBits: 8 });
-}, /^Error: Init error$/);
+if (process.versions.zlib.match(/^\d+\.\d+\.[9]|\d{2,}$/)) {


I think it would be good to include a link to the reasoning in zlib for this directly rather than having people blame and dig the link from the PR - but I'm fine with other way.

Good point, let me add a comment and see what you think. Thanks

Could you use test() instead of match(). And +1 to adding the comment.

cjihrig · 2017-06-15T16:49:30Z

-assert.throws(() => {
- zlib.createDeflateRaw({ windowBits: 8 });
-}, /^Error: Init error$/);
+if (process.versions.zlib.match(/^\d+\.\d+\.[9]|\d{2,}$/)) {


Could you use test() instead of match(). And +1 to adding the comment.

danbev · 2017-06-15T17:48:32Z

Could you use test() instead of match()

No problem, I'll change this.

jasnell

LGTM once the comment is added.

richardlau · 2017-06-15T20:00:25Z

+// This check was introduced in vesion 1.2.9 and prior to that there was
+// no such rejection which is the reason for the version check below
+// (http://zlib.net/ChangeLog.txt).
+if (/^\d+\.\d+\.[9]|\d{2,}$/.test(process.versions.zlib)) {


Is this regexp correct? What about a theoretical zlib 1.3.0?

It could probably be made stricter, yes. I also don’t quit get the |\d{2,}$ part – are there missing parentheses? /^a|b$/ is grouped as /(^a)|(b$)/, not /^(a|b)$/, but you probably meant something like the latter?

@richardlau You are right, I missed that :(
@addaleax @richardlau I think I went at this the wrong way trying to match allowed versions. I hope I've managed to simplify the expression by trying to detect an invalid version instead. Let me know what you think.

addaleax · 2017-06-16T12:59:40Z

+// This check was introduced in vesion 1.2.9 and prior to that there was
+// no such rejection which is the reason for the version check below
+// (http://zlib.net/ChangeLog.txt).
+if ((/^(?!1\.2\.[0-8]$)/.test(process.versions.zlib))) {


Isn’t this equivalent to (!/^1\.2\.[0-8]$/.test(process.versions.zlib))? That would look a bit simpler to me.

Yeah, I was choosing between the both and had a trouble making up my mind. I'd be happy to change it.

danbev · 2017-06-18T09:33:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8721/

richardlau

LGTM with one nit.

richardlau · 2017-06-18T09:36:19Z

- zlib.createDeflateRaw({ windowBits: 8 });
-}, /^Error: Init error$/);
+// invalid by zlib (http://zlib.net/manual.html#Advanced).
+// This check was introduced in vesion 1.2.9 and prior to that there was


Thanks, I'll fix this.

We are currenly builing Node with --shared-zlib which happens to be version 1.2.8. The test for zlib.createDeflateRaw is expected to fail but does not when using version 1.2.8. As far as I can tell the fix referred to in the comments was introduced in version 1.2.9: - Reject a window size of 256 bytes if not using the zlib wrapper This commit suggests adding a check for the version and skipping this assert if the version is less than 1.2.9. Refs: http://zlib.net/ChangeLog.txt PR-URL: nodejs#13697 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

danbev · 2017-06-18T10:37:32Z

Landed in 5189857

We are currenly builing Node with --shared-zlib which happens to be version 1.2.8. The test for zlib.createDeflateRaw is expected to fail but does not when using version 1.2.8. As far as I can tell the fix referred to in the comments was introduced in version 1.2.9: - Reject a window size of 256 bytes if not using the zlib wrapper This commit suggests adding a check for the version and skipping this assert if the version is less than 1.2.9. Refs: http://zlib.net/ChangeLog.txt PR-URL: #13697 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

MylesBorins · 2017-07-17T18:39:12Z

should this be backported to v6.x?

danbev · 2017-08-15T06:21:35Z

should this be backported to v6.x?

I don't think this needs to be backported. Sorry again about the late/missed reply on this.

drewfish · 2017-09-13T21:26:25Z

BTW here at Yahoo we just backported this to our internal v6.x branch, since we ./configure --shared-zlib with an older version of zlib.

MylesBorins · 2017-09-14T00:27:05Z

@drewfish would you be willing to submit your patch as a backport to v6.x-staging? There are a ton of conflicts rn, but I'm sure we would be willing to land it so you don't need to float it.

Also fwiw... if you are floating any other patches on 6.x please feel free to open an issue on https://github.com/nodejs/lts for us to take a look

MylesBorins · 2017-09-19T15:59:27Z

ping @drewfish

drewfish · 2017-09-19T20:43:34Z

pong #15478

We are currenly builing Node with --shared-zlib which happens to be version 1.2.8. The test for zlib.createDeflateRaw is expected to fail but does not when using version 1.2.8. As far as I can tell the fix referred to in the comments was introduced in version 1.2.9: - Reject a window size of 256 bytes if not using the zlib wrapper This commit suggests adding a check for the version and skipping this assert if the version is less than 1.2.9. Refs: http://zlib.net/ChangeLog.txt Backport-PR-URL: #15478 PR-URL: #13697 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 15, 2017

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Jun 15, 2017

benjamingr approved these changes Jun 15, 2017
View reviewed changes

cjihrig approved these changes Jun 15, 2017
View reviewed changes

jasnell approved these changes Jun 15, 2017
View reviewed changes

danbev added 2 commits June 15, 2017 20:36

add comment about the version check
7e61bcf

use test() instead of match()
a6365bf

richardlau reviewed Jun 15, 2017
View reviewed changes

update regex to check for invalid version
b9833a1

lpinca approved these changes Jun 16, 2017
View reviewed changes

addaleax approved these changes Jun 16, 2017
View reviewed changes

simplify regex
aa525e4

richardlau approved these changes Jun 18, 2017
View reviewed changes

fix spelling of version
1441b21

danbev closed this Jun 18, 2017

addaleax mentioned this pull request Jun 21, 2017
v8.2.0 proposal #13744
Merged

addaleax mentioned this pull request Jun 21, 2017
v8.1.3 proposal #13861
Merged

danbev deleted the zlib-version-check-test branch June 28, 2017 05:47

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added the dont-land-on-v6.x label Aug 14, 2017

MylesBorins removed the lts-watch-v6.x label Aug 14, 2017

MylesBorins added lts-watch-v6.x and removed dont-land-on-v6.x labels Sep 14, 2017

MylesBorins added backport-requested-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Sep 22, 2017

MylesBorins mentioned this pull request Sep 22, 2017
v6.11.4 proposal #15506
Merged

Uh oh!

Uh oh!

Conversation

danbev commented Jun 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Jun 15, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Jun 18, 2017

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Jun 18, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

danbev commented Aug 15, 2017

Uh oh!

drewfish commented Sep 13, 2017

Uh oh!

MylesBorins commented Sep 14, 2017

Uh oh!

MylesBorins commented Sep 19, 2017

Uh oh!

drewfish commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants