On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <i...@visualsvn.com> wrote:
> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rpl...@apache.org> 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 <ylavic....@gmail.com> wrote: >> >> >> >> Hi Ivan, >> >> >> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <i...@visualsvn.com> 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 What do you think? It would be great if someone could take a look on the implementation from the *nix perspective. After that, I propose to merge the branch into trunk. (I will be travelling/on vacation for a couple of weeks, so might not be able to answer promptly.) -- Ivan Zhakov
