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 4. Backport it to 1.7.x/1.8.x And if this plan makes sense, I am ready to proceed with steps (1), (3) and (4). -- Ivan Zhakov
