Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119517
MIME-Version: 1.0
References: <Y+TIlliuIwRyZX8L@swift.blarg.de>
In-Reply-To: <Y+TIlliuIwRyZX8L@swift.blarg.de>
Date: Thu, 9 Feb 2023 21:18:08 +0000
Message-ID: <CAEKnhAEAwMEh4unOfJkzrTF9PkhHvAniHOUXhaoB4YNdpUZsgA@mail.gmail.com>
To: Max Kellermann <max+php@blarg.de>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary="00000000000088008705f44aeb6c"
Subject: Re: [PHP-DEV] How to deal with bugs in vendored libraries?
From: bukka@php.net (Jakub Zelenka)

--00000000000088008705f44aeb6c
Content-Type: text/plain; charset="UTF-8"

Hi,

On Thu, Feb 9, 2023 at 10:19 AM Max Kellermann <max+php@blarg.de> wrote:

> Hi,
>
> what happens if there is a bug in a vendored library, but upstream
> refuses to fix it?
>
> Last month, my PR for removing obsolete C99 integer checks was merged:
>
>  https://github.com/php/php-src/pull/10304
>
> This change speeds up configure and removes code that violates the C99
> spec.
>
> It included a fix for ext/date/lib/ which I then learned is a vendored
> library; Christoph M. Becker asked me to submit the same fix upstream,
> which I did:
>
>  https://github.com/derickr/timelib/pull/141
>
> As you can see, the PR was closed with a comment that did not explain
> why.  After it was pointed out to them that the existing code violates
> the C99 standard, they locked the PR, preventing further discussion.
>
> Not only that; they also reverted my fix in php-src, reintroducing the
> C99 spec violation:
>
>  https://github.com/php/php-src/commit/0df92d218e88a0070fcebd5391e7
>
> ... even though UPGRADING.INTERNALS contains an explanation that these
> obsolete feature macros are no longer present in PHP's config headers
> and should not be used.
>
> Two questions remain for me:
>
> - Should somebody with push access to php-src silently/secretly,
>   without any discussion, revert such a fix?  The revert seems to have
>   been directly pushed to php-src without a PR and with no chance for
>   others to review it.
>
>
Yes if it's a maintainer of the code which is the case here. Historically
we didn't use PR's for normal fixes. We started doing so mainly to run
tests rather than getting feedback. That said I think it's good to do PR's
for most changes as it's safer and feedback is good but it has never been a
requirement.


> - In general, if a vendored library has a bug (like the C99 spec
>   violation, or any other bug) that the upstream refuses to fix or
>   even discuss, shall the copy in php-src be patched?  Are deviations
>   from vanilla upstream possible?  Of course, this causes extra work
>   for syncing with new upstream released, but tools like quilt could
>   help with that.
>
>
I don't think we would want to do this in general unless it's something
critical or security related which this is certainly not.

The only thing that you can do here is to create an RFC but I'm pretty sure
that it would be just waste of time as it would be destined to fail. So the
best thing that you can do is to just forget about it and move on otherwise
I think you are wasting your time...

Regards

Jakub

--00000000000088008705f44aeb6c--