On Thu, 20 Jan 2022 at 17:39, Ivan Zhakov <[email protected]> wrote:
> On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <[email protected]> wrote: > >> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <[email protected]> wrote: >> >>> >>> >>> On 1/13/22 7:04 PM, Ivan Zhakov wrote: >>> > [[ sorry for delayed response ]] >>> > >>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <[email protected]> wrote: >>> >> >>> >> Hi Ivan, >>> >> >>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <[email protected]> >>> wrote: >>> >>> >>> >>> This change does not compile on Windows in APR 1.7.x: >>> >>> [[[ >>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared >>> identifier >>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' >>> must >>> >>> point to struct/union >>> >> >>> >> I was missing backport of r1895178, does r1896808 compile now? >>> >> (Sorry, no Windows at hand..). >>> > Yes, it builds now. Thanks! >>> > >>> >> >>> >>> >>> >>> I also have a high-level objection against backporting this change to >>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only >>> >>> regression fixes should be backported to the stable branch. r1896510 >>> >>> is a significant change and as far as I understand it's not a >>> >>> regression fix. So I think it would be better to revert r1896510 and >>> >>> release it as part of APR 2.0 (or 1.8.x). >>> >> >>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are >>> >> fixes for bugs that were there before 1.7 already, not regressions >>> >> introduced by 1.7.0. >>> > >>> > Agreed on the bugfix/regressions part. I have misunderstood that >>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that >>> > it adds new functionality. But even with that in mind, I still think >>> > that the size of the change might be just too large for it to be an >>> > appropriate fit for a patch release. >>> > >>> > Speaking of the change itself, I think that there might be an >>> > alternative to making the apr_file_t also handle sockets on Windows. >>> > It might be better to specifically change the pollset implementation >>> > so that on Windows it would add a socket and use it for wakeup, >>> > instead of using the socket disguised as a file. >>> > >>> > If this alternative approach sounds fine, I could try to implement it. >>> >>> But this could wait for a 1.7.2, correct? I am asking because there is >>> some desire to get 1.7.1 out of the door soon. >>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and >>> is released soon after 1.7.2. >>> >>> 1. Revert this change from 1.7.x >> 2. Release 1.7.1 >> 3. Rework this code on trunk without changing the apr_file_t's behavior >> > This part is now in the following branch: > > https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation > > Sorry, wrong branch URL. The correct URL is: https://svn.apache.org/repos/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation -- Ivan Zhakov
