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

deps: upgrade zlib to 1.2.11 #10980

Merged
merged 1 commit into from Feb 20, 2017

Conversation

Projects
None yet
@sam-github
Member

sam-github commented Jan 24, 2017

Update zlib.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 24, 2017

Member

No idea why the bot thinks this shouldn't land on 7.x, it cherry-picked clean for me onto 7.x staging.

Member

sam-github commented Jan 24, 2017

No idea why the bot thinks this shouldn't land on 7.x, it cherry-picked clean for me onto 7.x staging.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 24, 2017

Contributor

Yeah I think something has been wrong with the bot for awhile now, I've had to manually un-label that countless times.

Contributor

mscdex commented Jan 24, 2017

Yeah I think something has been wrong with the bot for awhile now, I've had to manually un-label that countless times.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 24, 2017

Member

I thought the bot just didn't take into account that a PR would land as long as earlier PRs landed first - but this change depends on nothing, so it looks like its quite broken.

Member

sam-github commented Jan 24, 2017

I thought the bot just didn't take into account that a PR would land as long as earlier PRs landed first - but this change depends on nothing, so it looks like its quite broken.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jan 24, 2017

Member

The fips buildbot failed to download https://openssl.org/source/openssl-fips-2.0.9.tar.gz.

@nodejs/build Can we cache it somewhere? It's a point of failure and not very good netizenship to download it with every CI run.

Member

bnoordhuis commented Jan 24, 2017

The fips buildbot failed to download https://openssl.org/source/openssl-fips-2.0.9.tar.gz.

@nodejs/build Can we cache it somewhere? It's a point of failure and not very good netizenship to download it with every CI run.

@bnoordhuis

Rubber-stamp LGTM. You might be able to shrink the diff if you dos2unix the files first.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 25, 2017

Member

Thanks, @bnoordhuis, done.

I suppose zlib to be pretty stable, are the clean test results sufficient to land this, or is there anything else that can be done to increase confidence?

Member

sam-github commented Jan 25, 2017

Thanks, @bnoordhuis, done.

I suppose zlib to be pretty stable, are the clean test results sufficient to land this, or is there anything else that can be done to increase confidence?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jan 25, 2017

Member

A green CI should be good enough.

Member

bnoordhuis commented Jan 25, 2017

A green CI should be good enough.

@gibfahn gibfahn referenced this pull request Jan 25, 2017

Open

Cache CI downloads #599

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 25, 2017

Member

@nodejs/ctc some more thumbs up please, and I'll land tomorrow.

/cc @MylesBorins

Member

sam-github commented Jan 25, 2017

@nodejs/ctc some more thumbs up please, and I'll land tomorrow.

/cc @MylesBorins

deflateGetDictionary returns Z_OK on success, or Z_STREAM_ERROR if the
stream state is inconsistent.
*/

This comment has been minimized.

@shigeki

shigeki Jan 26, 2017

Contributor

Does this correspond to semver-minor? Looking at other diffs in zlib.h, there seems API/ABI compatible fortunately.

@shigeki

shigeki Jan 26, 2017

Contributor

Does this correspond to semver-minor? Looking at other diffs in zlib.h, there seems API/ABI compatible fortunately.

This comment has been minimized.

@sam-github

sam-github Jan 27, 2017

Member

You believe that it is ABI compatible? Do we have a way of testing this? @MylesBorins you mentioned an "ABI roaster"?

@sam-github

sam-github Jan 27, 2017

Member

You believe that it is ABI compatible? Do we have a way of testing this? @MylesBorins you mentioned an "ABI roaster"?

This comment has been minimized.

@shigeki

shigeki Jan 31, 2017

Contributor

You believe that it is ABI compatible?

As far as, I look at this but I do not have a confidence.

Do we have a way of testing this? @MylesBorins you mentioned an "ABI roaster"?

I'm not sure. It is nice to have a tools to check it.

@shigeki

shigeki Jan 31, 2017

Contributor

You believe that it is ABI compatible?

As far as, I look at this but I do not have a confidence.

Do we have a way of testing this? @MylesBorins you mentioned an "ABI roaster"?

I'm not sure. It is nice to have a tools to check it.

@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki Jan 26, 2017

Contributor

I found that I had missed to add -DHAVE_HIDDEN in zlib.gyp when gcc is used in the last upgrade . https://github.com/nodejs/node/blob/master/deps/zlib/configure#L735. Do we need this?

Contributor

shigeki commented Jan 26, 2017

I found that I had missed to add -DHAVE_HIDDEN in zlib.gyp when gcc is used in the last upgrade . https://github.com/nodejs/node/blob/master/deps/zlib/configure#L735. Do we need this?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 27, 2017

Member

re -DHAVE_HIDDEN, would adding this to zlib.gyp make the ABI different? It looks like it would.

Member

sam-github commented Jan 27, 2017

re -DHAVE_HIDDEN, would adding this to zlib.gyp make the ABI different? It looks like it would.

@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki Jan 31, 2017

Contributor

HAVE_HIDDEN is defined by default in zlib when gcc is used in order to hide internal functions. According to follow the upstream preferences, I think it would be better to add it.

Contributor

shigeki commented Jan 31, 2017

HAVE_HIDDEN is defined by default in zlib when gcc is used in order to hide internal functions. According to follow the upstream preferences, I think it would be better to add it.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 31, 2017

Member

I agree it would be better to define HAVE_HIDDEN, but I think changing the visibility of symbols is unrelated to this PR to update the source, and since symbol visibility effects the API, it would be semver-major.

Can I open a seperate PR for HAVE_HIDDEN?

Member

sam-github commented Jan 31, 2017

I agree it would be better to define HAVE_HIDDEN, but I think changing the visibility of symbols is unrelated to this PR to update the source, and since symbol visibility effects the API, it would be semver-major.

Can I open a seperate PR for HAVE_HIDDEN?

@shigeki

shigeki approved these changes Feb 1, 2017

@shigeki

This comment has been minimized.

Show comment
Hide comment
@shigeki

shigeki Feb 1, 2017

Contributor

Can I open a seperate PR for HAVE_HIDDEN?

Sure, let's move it.

Contributor

shigeki commented Feb 1, 2017

Can I open a seperate PR for HAVE_HIDDEN?

Sure, let's move it.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 1, 2017

Member

HAVE_HIDDEN: #11082

Member

sam-github commented Feb 1, 2017

HAVE_HIDDEN: #11082

@sam-github

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

#11082 has landed

Member

jasnell commented Feb 2, 2017

#11082 has landed

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 17, 2017

Member

What's the status on this one? :-)

Member

jasnell commented Feb 17, 2017

What's the status on this one? :-)

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 20, 2017

Member

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

I'll merge after a final confirmation its passing.

Member

sam-github commented Feb 20, 2017

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

I'll merge after a final confirmation its passing.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 20, 2017

Member

Z is having difficulty in git checkout, twice in a row, latest is https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel72-s390x/4075/console

Member

sam-github commented Feb 20, 2017

Z is having difficulty in git checkout, twice in a row, latest is https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel72-s390x/4075/console

@sam-github

This comment has been minimized.

Show comment
Hide comment
deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

@sam-github sam-github merged commit dfa8abe into nodejs:master Feb 20, 2017

@sam-github sam-github deleted the sam-github:update-zlib branch Feb 20, 2017

addaleax added a commit that referenced this pull request Feb 21, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Mar 21, 2017

Member

@nodejs/lts this is patch, not semver-minor, so it isn't on nodejs/Release#188, but it doesn't seem to be picked into v6.x-staging or v4.x-staging

How do I track the decision making process? How does this get onto the LTS meeting agenda?

Member

sam-github commented Mar 21, 2017

@nodejs/lts this is patch, not semver-minor, so it isn't on nodejs/Release#188, but it doesn't seem to be picked into v6.x-staging or v4.x-staging

How do I track the decision making process? How does this get onto the LTS meeting agenda?

@gibfahn gibfahn referenced this pull request Mar 21, 2017

Closed

Potential Semver Minor Backports #188

28 of 31 tasks complete

MylesBorins added a commit that referenced this pull request Mar 22, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins added a commit that referenced this pull request Mar 22, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Mar 22, 2017

Member

I've landed this in both v4.x and v6.x staging... not sure how we missed it in the last audit.

Member

MylesBorins commented Mar 22, 2017

I've landed this in both v4.x and v6.x staging... not sure how we missed it in the last audit.

MylesBorins added a commit that referenced this pull request Mar 22, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins added a commit that referenced this pull request Mar 29, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins added a commit that referenced this pull request Mar 29, 2017

deps: upgrade zlib to 1.2.11
PR-URL: #10980
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins added a commit that referenced this pull request Mar 29, 2017

2017-04-04, Version 4.8.2 'Argon' (LTS)
This is a special LTS to fix a memory leak that was introduced
in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980

@MylesBorins MylesBorins referenced this pull request Mar 29, 2017

Merged

V6.10.2 proposal #12128

MylesBorins added a commit that referenced this pull request Mar 29, 2017

2017-04-04, Version 6.10.2 'Boron' (LTS)
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123

@MylesBorins MylesBorins referenced this pull request Mar 29, 2017

Merged

v4.8.2 proposal #12129

MylesBorins added a commit that referenced this pull request Apr 4, 2017

2017-04-04, Version 4.8.2 'Argon' (Maintenance)
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980

MylesBorins added a commit that referenced this pull request Apr 4, 2017

2017-04-04, Version 6.10.2 'Boron' (LTS)
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123

MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017

2017-04-04, Version 6.10.2 'Boron' (LTS)
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) nodejs#12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    nodejs#12123

MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017

2017-04-04, Version 4.8.2 'Argon' (Maintenance)
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
#ifdef __50SERIES /* Prime/PRIMOS */
# define OS_CODE 0x0f
#ifdef __APPLE__
# define OS_CODE 19

This comment has been minimized.

@rmg

rmg Apr 5, 2017

Contributor

This might be the source of #12244. This would slightly change the gzip header that is produced by zlib.gzip() on macOS. From looking at the code it seems that macOS would have previously fallen through to the default of 0x03 (Unix).

@rmg

rmg Apr 5, 2017

Contributor

This might be the source of #12244. This would slightly change the gzip header that is produced by zlib.gzip() on macOS. From looking at the code it seems that macOS would have previously fallen through to the default of 0x03 (Unix).

This comment has been minimized.

@MylesBorins

MylesBorins Apr 6, 2017

Member

is this something we would want to float a patch for on LTS?

@MylesBorins

MylesBorins Apr 6, 2017

Member

is this something we would want to float a patch for on LTS?

This comment has been minimized.

@sam-github

sam-github Apr 6, 2017

Member

I am on the fence. Its clearly an observable change, @flippmoke saw it, but its not clear that we are that committed to binary identicality of output encoding. Then again, it seems a low-risk change to float a patch on the LTS branches to make them absolutely identical in output. @flippmoke, do you have an opinion on this? Do you feel that your test suites were depending on unstable encoding details, or do you feel that this is a regression?

@sam-github

sam-github Apr 6, 2017

Member

I am on the fence. Its clearly an observable change, @flippmoke saw it, but its not clear that we are that committed to binary identicality of output encoding. Then again, it seems a low-risk change to float a patch on the LTS branches to make them absolutely identical in output. @flippmoke, do you have an opinion on this? Do you feel that your test suites were depending on unstable encoding details, or do you feel that this is a regression?

This comment has been minimized.

@flippmoke

flippmoke Apr 6, 2017

@sam-github I would say we were surprised to see a in encoding change due to a patch release. However, I can understand how this is not a big deal for the vast majority of use cases for users. It is possibly confusing as well for OSX users that might use zlib elsewhere that they will not get the same binary results. However, I feel this is more an issue with zlib issuing this in a patch release rather then node doing this in a patch release.

Also just a note I am somewhat confused by the MAC OS header above it as well and when _ _ APPLE _ _applies compared to it. https://github.com/nodejs/node/pull/10980/files/dfa8abe1b500b86ffb3951736c209e57d83ed3ed#diff-a12b3cb4ae22eb7956dc70bda62773a3L121

@flippmoke

flippmoke Apr 6, 2017

@sam-github I would say we were surprised to see a in encoding change due to a patch release. However, I can understand how this is not a big deal for the vast majority of use cases for users. It is possibly confusing as well for OSX users that might use zlib elsewhere that they will not get the same binary results. However, I feel this is more an issue with zlib issuing this in a patch release rather then node doing this in a patch release.

Also just a note I am somewhat confused by the MAC OS header above it as well and when _ _ APPLE _ _applies compared to it. https://github.com/nodejs/node/pull/10980/files/dfa8abe1b500b86ffb3951736c209e57d83ed3ed#diff-a12b3cb4ae22eb7956dc70bda62773a3L121

This comment has been minimized.

@rmg

rmg Apr 11, 2017

Contributor

@flippmoke I believe that is a relic of the pre-xcode days when MetroWerks/CodeWarrior was the compiler of choice on Mac. Yay legacy!

@rmg

rmg Apr 11, 2017

Contributor

@flippmoke I believe that is a relic of the pre-xcode days when MetroWerks/CodeWarrior was the compiler of choice on Mac. Yay legacy!

imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017

2017-04-04, Version 4.8.2 'Argon' (Maintenance)
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017

2017-04-04, Version 6.10.2 'Boron' (LTS)
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017

2017-04-04, Version 4.8.2 'Argon' (Maintenance)
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017

2017-04-04, Version 6.10.2 'Boron' (LTS)
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Jul 20, 2017

Hi, i suspect regression. I have pako, zlib port to js, and it uses node's zlib to verify data. After 6.10.2 tests fails with deflate { level: 0 }

See https://travis-ci.org/nodeca/pako/jobs/255485506.

To reproduce - clone paco's repo, and run tests in desired version of node. 6.10.1 works, all next - not.

Can anyone comment?

puzrin commented Jul 20, 2017

Hi, i suspect regression. I have pako, zlib port to js, and it uses node's zlib to verify data. After 6.10.2 tests fails with deflate { level: 0 }

See https://travis-ci.org/nodeca/pako/jobs/255485506.

To reproduce - clone paco's repo, and run tests in desired version of node. 6.10.1 works, all next - not.

Can anyone comment?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 21, 2017

Member

The binary output format changed, particularly for OS X, is there any chance you have assertions based on the zipped binary data? I cloned pako and did an npm test on Linux and it passed with node 8.1.4, are you testing on OS X? That would be a smoking gun, see #12244 and #12404

Member

sam-github commented Jul 21, 2017

The binary output format changed, particularly for OS X, is there any chance you have assertions based on the zipped binary data? I cloned pako and did an npm test on Linux and it passed with node 8.1.4, are you testing on OS X? That would be a smoking gun, see #12244 and #12404

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Jul 21, 2017

is there any chance you have assertions based on the zipped binary data

That's exactly what i do. I use pako & node's zlib with all possible options and check that results are binary equal https://github.com/nodeca/pako/blob/master/test/deflate.js

Also note, that in my case it fails with { level: 0 } - when no compression applied and input data pass through almost as is (but with header + crc, except raw deflate)

I cloned pako and did an npm test on Linux and it passed with node 8.1.4

Sorry i've just disabled broken tests. If you enable those back, tests will fail in all nodes with zlib 1.2.11.

I use Ubuntu 14.04lts

About os type in header (pako writes "unix") - that make sense for gzip only, not for deflate.

puzrin commented Jul 21, 2017

is there any chance you have assertions based on the zipped binary data

That's exactly what i do. I use pako & node's zlib with all possible options and check that results are binary equal https://github.com/nodeca/pako/blob/master/test/deflate.js

Also note, that in my case it fails with { level: 0 } - when no compression applied and input data pass through almost as is (but with header + crc, except raw deflate)

I cloned pako and did an npm test on Linux and it passed with node 8.1.4

Sorry i've just disabled broken tests. If you enable those back, tests will fail in all nodes with zlib 1.2.11.

I use Ubuntu 14.04lts

About os type in header (pako writes "unix") - that make sense for gzip only, not for deflate.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 21, 2017

Member

I'll run again just to verify, but if you think this is a bug, can you provide a standalone repro?

But, if you are doing things like the following, you might want to consider whether it is a valid test:

p = plain text
pako = pako.compress(p)
node = node.compress(p)
assert(pako == node)

I don't think above is. There are usually multiple legal compressed forms, what the algorithms guarantee is: orig = uncompress(compress(orig)), they don't guarantee the compressed form.

p = plain text
pako = pako.compress(p)
node = node.compress(p)
assert(pako.uncompress(node) == p)
assert(node.uncompress(pako) == p)

Again, I haven't read your test suite, so above is just a note about things that we've seen a few people have problems with in their test suites (not applications) after the zlib update.

Member

sam-github commented Jul 21, 2017

I'll run again just to verify, but if you think this is a bug, can you provide a standalone repro?

But, if you are doing things like the following, you might want to consider whether it is a valid test:

p = plain text
pako = pako.compress(p)
node = node.compress(p)
assert(pako == node)

I don't think above is. There are usually multiple legal compressed forms, what the algorithms guarantee is: orig = uncompress(compress(orig)), they don't guarantee the compressed form.

p = plain text
pako = pako.compress(p)
node = node.compress(p)
assert(pako.uncompress(node) == p)
assert(node.uncompress(pako) == p)

Again, I haven't read your test suite, so above is just a note about things that we've seen a few people have problems with in their test suites (not applications) after the zlib update.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Jul 21, 2017

I'll run again just to verify, but if you think this is a bug, can you provide a standalone repro?

I do not know, is it a bug or not. But pako is not "yet another deflate implementation", it's a real port of zlib 1.2.8. And i suspect, zlib did not changed binary output (i don't count 1 byte of OS code in header). So, prior to dig deep, i asked, if anybody know about similar issues.

I'll triage in 1-2 weeks and tell you result.

puzrin commented Jul 21, 2017

I'll run again just to verify, but if you think this is a bug, can you provide a standalone repro?

I do not know, is it a bug or not. But pako is not "yet another deflate implementation", it's a real port of zlib 1.2.8. And i suspect, zlib did not changed binary output (i don't count 1 byte of OS code in header). So, prior to dig deep, i asked, if anybody know about similar issues.

I'll triage in 1-2 weeks and tell you result.

@puzrin

This comment has been minimized.

Show comment
Hide comment
@puzrin

puzrin Jul 21, 2017

@sam-github https://github.com/puzrin/node_zlib1211_l0

It seems, zlib really changed binary representation of deflate "stored" mode. That's very strange but i don't consider it as bug.

puzrin commented Jul 21, 2017

@sam-github https://github.com/puzrin/node_zlib1211_l0

It seems, zlib really changed binary representation of deflate "stored" mode. That's very strange but i don't consider it as bug.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jul 21, 2017

Member

@puzrin Thanks for looking into this. I don't regard it as a bug either.

Member

sam-github commented Jul 21, 2017

@puzrin Thanks for looking into this. I don't regard it as a bug either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment