Skip to content
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

Revert "Make a noble effort at setting OS_CODE correctly." #12404

Closed

Conversation

sam-github
Copy link
Contributor

Revert change to the OS_CODE in the zlib header made in zlib 1.2.11. It
changes the binary format of the compressed data on MacOS. This has been
found to cause problems for test suites that used to be passing in 4.x
and 6.x, and that had assertions based on the expect compressed output.

Refs: madler/zlib@ce12c5c

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Revert change to the OS_CODE in the zlib header made in zlib 1.2.11. It
changes the binary format of the compressed data on MacOS. This has been
found to cause problems for test suites that used to be passing in 4.x
and 6.x, and that had assertions based on the expect compressed output.

Refs: madler/zlib@ce12c5c
@nodejs-github-bot nodejs-github-bot added v6.x zlib Issues and PRs related to the zlib subsystem. labels Apr 13, 2017
@sam-github
Copy link
Contributor Author

Haven't tested on OS X yet, so its WIP, should get done today.

@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

Passed by hand on os x and linux, failed on os x without the revert patch.

@bnoordhuis
Copy link
Member

I'm mildly -1 on this. For one, the test can fail when built against a shared zlib. For two, chopping up our in-tree copy like that is just... icky. I don't like the idea of floating patches that aren't strictly necessary.

@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

Definitely a grey area. I don't think people should be relying on the output encoding, but we've found at least two instances where they were. I can let this sit and we can wait for more bug reports.

@jasnell
Copy link
Member

jasnell commented Apr 14, 2017

I'm leaning in the same direction as @bnoordhuis on this.

@sam-github
Copy link
Contributor Author

/fyi @rmg

@rmg
Copy link
Contributor

rmg commented Apr 18, 2017

I was willing to live with the yuck factor if it meant giving users a better experience, but the fact that this was already "broken" if someone switched between builtin and shared zlib is an even better argument.

I think the problems caused have been too few to warrant the cost of this bug compatibility. I would say sit on it for a little while longer, but it has already been 2 weeks since the initial report and there has only been 1 other instance in that time.

-1, we can re-open if we suddenly get a flood of new reports.

@MylesBorins
Copy link
Contributor

Closing for now

@sam-github sam-github deleted the revert-os-zlib-header-change branch April 19, 2017 18:04
@sam-github sam-github mentioned this pull request Jul 21, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants