This only manifests with the zlib update patch applied.

My suggestion is to first get the update done and after it is landed, we could come back and investigate what might be wrong in the buildsystem for OSX.

I'm trying to move to sharing zlib with Chromium and I'm not seeing this in https://pdfium-review.googlesource.com/c/3350/

This will only appear while using the build system of PDFium.

I'm not seeing it though. I effectively dropped this custom change in the CL I mentioned and it builds fine on the PDFium trybots.

Yes, as expected you won't see this using Chromium's zlib.

First, we got to define clearly what has changed on your CL.

If I understood correctly, by using DEPS you will be checking the source + build files from third_party/zlib.

Therefore, you will perform the build of zlib using the build rules and the zconf.h from there, instead of the ones in PDFium.

As this is pretty clear, the answer for understanding *why* this won't show up should be simply a matter of diffing both zconf headers.

A diff shows this difference:
https://gist.github.com/Adenilson/2587521904fa36820add9c61fa8f323d

In linux that is not an issue because the compiler will include unistd.h from other place than in OSX.

So instead of the OSX specific workaround I did when we first landed
the update to zlib 1.2.11, an even better fix would be simply have a #define Z_HAVE_UNISTD_H in zconf.h in PDFium.

I'm planning to do this when we have done the zlib_namespace refactoring.

Chromium's zlib has been patched with google.patch, and it works on all platforms supported by Chromium AFAIK. If PDFium pulls that in and delete PDFium's bundled zlib, we get the same benefit and this bug goes away. Isn't that better than trying to fix PDFium's zlib?

It is relevant to mention that the aforementioned change in Chromium's zlib is a patch on top of the vanilla zlib.

For reference:
a) Vanilla: https://github.com/madler/zlib/blob/master/zconf.h#L434

b) Chromium: https://cs.chromium.org/chromium/src/third_party/zlib/zconf.h?q=zconf.h+package:%5Echromium$&dr=CSs&l=437

In a second thought, it is not advisable to keep forking vanilla zlib, thus the approach in Chromium's zlib is suboptimal.

> Isn't that better than trying to fix PDFium's zlib?

As I'm a newcomer in PDFium, I prefer to ask for input from their maintainers.
:-)

@dsinclair and npm, any thoughts?

Yes, I already acknowledged Chromium's zlib is patched in comment 11. Chromium has always used its own copy of zlib since day 1. Why do you say "it is not advisable to keep forking vanilla zlib" ? On Windows, there is no system zlib. Chromium needs its own copy. Let's assume Chromium is going to keep using its own zlib, because it's been that way for almost 10 years and it probably won't change. The topic of using system zlib on Linux/Mac vs Chromium's bundled copy is a separate discussion that's way out of the scope of this bug. 

Hey, I'm a PDFium maintainer too. :-P

> Yes, I already acknowledged Chromium's zlib is patched in comment 11.

You posted while I was writing my message. As I had a previous version of the page, your later comments weren't visible (notice that there is a 1-2 minutes difference between the comments).

> Isn't that better than trying to fix PDFium's zlib?

I'm trying to fix PDFium's zlib as a first step towards using Chromium's zlib.

Understand that we have the same shared goals.


> Chromium has always used its own copy of zlib since day 1.

Which is pretty bad.

You may not be aware of this ongoing effort:
https://bugs.chromium.org/p/chromium/issues/detail?id=687631

Basically the master plan is for Chromium to use a vanilla zlib and have all the optimizations upstreamed.

In an ideal world, even chromium's zlib could become a DEP.

> been that way for almost 10 years and it probably won't change.

Hopefully this will change for the better.
:-)

> Hey, I'm a PDFium maintainer too. :-P

Awesome! PDFium is a pretty cool project that I'm looking forward to start contributing.

We have the same goals, but in my CL, it looks like jumping straight to Chromium's zlib works. So why not just do that?

I'm glad to see we are working on upstreaming some of the optimizations. But even then, in the Chromium checkout, we need a copy of zlib. If that comes via DEPS instead of being checked in, then sure, let's do that. However, that doesn't change the discussion here and change the fact that Chromium's copy still need to keep its own symbol namespace even if it pulls in most of the source from upstream. PDFium's standalone build should just follow whatever Chromium does.

>However, that doesn't change the discussion here and change the fact that >Chromium's copy still need to keep its own symbol namespace even if it pulls in >most of the source from upstream.

I have one idea about how to handle that without patching zlib that I'm discussing with zlib-ng developers.

> PDFium's standalone build should just follow whatever Chromium does.

For sure that would make things much easier.

My understanding was that the local changes were a *must have*. If they are *not*, that would be awesome.

Please look take a look at https://pdfium-review.googlesource.com/c/3350/ where I explained my thoughts on why all the local changes can be dropped.

As PDFium now is using Chromium's zlib, this is no longer an issue.