test: check zlib version for createDeflateRaw #13697

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

test/parallel/test-zlib-failed-init.js

-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

test/parallel/test-zlib-failed-init.js

-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

test/parallel/test-zlib-failed-init.js

+// 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

test/parallel/test-zlib-failed-init.js

+// 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

test/parallel/test-zlib-failed-init.js

-  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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: check zlib version for createDeflateRaw #13697

test: check zlib version for createDeflateRaw #13697

danbev commented Jun 15, 2017

danbev commented Jun 15, 2017

danbev commented Jun 15, 2017

benjamingr Jun 15, 2017

danbev Jun 15, 2017

cjihrig Jun 15, 2017

cjihrig Jun 15, 2017

danbev commented Jun 15, 2017

jasnell left a comment

richardlau Jun 15, 2017

addaleax Jun 15, 2017

danbev Jun 16, 2017

addaleax Jun 16, 2017

danbev Jun 16, 2017

danbev commented Jun 18, 2017

richardlau left a comment

richardlau Jun 18, 2017

danbev Jun 18, 2017

danbev commented Jun 18, 2017

MylesBorins commented Jul 17, 2017

danbev commented Aug 15, 2017

drewfish commented Sep 13, 2017

MylesBorins commented Sep 14, 2017

MylesBorins commented Sep 19, 2017

drewfish commented Sep 19, 2017

test: check zlib version for createDeflateRaw #13697

test: check zlib version for createDeflateRaw #13697

Conversation

danbev commented Jun 15, 2017

Checklist

Affected core subsystem(s)

danbev commented Jun 15, 2017

danbev commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Jun 15, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Jun 18, 2017

richardlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Jun 18, 2017

MylesBorins commented Jul 17, 2017

danbev commented Aug 15, 2017

drewfish commented Sep 13, 2017

MylesBorins commented Sep 14, 2017

MylesBorins commented Sep 19, 2017

drewfish commented Sep 19, 2017