Fix mingw-cross build

[kinkie]

cast from 'void*' HANDLE to long int loses precision [-fpermissive]

[kinkie]

note: mswindows.h changes are from the mingw-cross-1 branch . I'm not entirely sure how to stack PR on top of each other so that github only shows the current PR changes

[rousskov]

note: mswindows.h changes are from the mingw-cross-1 branch . I'm not entirely sure how to stack PR on top of each other so that github only shows the current PR changes

AFAIK, GitHub does not support such "stacked" PRs. GitHub PRs are based on git branches. GitHub computes PR branch forking point against PR target branch (i.e. the master branch in this case). The existence of other PRs/branches/etc. is absolutely irrelevant in this computation.

In general, reasonable choices include:

1. Create a dedicated branch for PR B, branching off the future target branch (i.e. the master branch in this case).
2. Wait for the PR A to merge before (creating the next PR B branch and) posting PR B.
3. Post a stacked PR B immediately but mark it as a Draft PR and explain that it contains PR A changes. The S-waiting-for-PR PR label may be useful in such cases! Be ready to forgive reviewers if they cannot review the resulting mess (correctly). After PR A merges, rebase the PR B branch, force push, and remove the Draft designation from PR B. It may also be possible to merge the target branch into PR B to avoid force-pushing.

All this requires a bit of extra work/pain. The extra cost increases PR minimization price, of course. In most cases, smaller dedicated PRs are still the lesser evil.

[kinkie]

My current plan with this stack:

1. get it to compile using mingw cross
2. run it, fix any issues
3. try porting to a native windows stack. I have a VM almost ready to do it and keep it sustainable

[kinkie]

note: mswindows.h changes are from the mingw-cross-1 branch . I'm not entirely sure how to stack PR on top of each other so that github only shows the current PR changes

AFAIK, GitHub does not support such "stacked" PRs.

[...] Pity; for $dayjob we use phabricator and it supports PR stacks, it's quite convenient.

All this requires a bit of extra work/pain. The extra cost increases PR minimization price, of course. In most cases, smaller dedicated PRs are still the lesser evil.

I agree. So, this current PR is targeted to fix compat/mswindows.* and it should be almost ready. Squid doesn't build yet due to problems in sspwin32.cc, squidclient.cc, Transport.cc, cachemgr.cc, ssl .

I'll tackle each of those with a dedicated PR, waiting for the previous one to be merged before moving on.

[rousskov]

we use phabricator and it supports PR stacks, it's quite convenient.

To avoid misunderstanding: GitHub supports a flavor of stacked PRs for same-repository branches. We use that feature internally quite often! However, here, we are dealing with a public Project repository, and GitHub (somewhat naturally) does not let you create PRs against branches that are not in the repository.

I'll tackle each of those with a dedicated PR, waiting for the previous one to be merged before moving on.

Good luck with making progress in this messy code!

Please note that it is not clear what the current S-waiting-for-reviewer label meant to indicate because there are two active reviewers and no reviews have been requested. To request review(s), use the Reviewers panel in the top right corner.

[kinkie]

This work is interim but it doesn't regress the general status and is a step forward to the intended goal. I'd like to merge it to checkpoint and start anew. The XXX @rousskov marked in the last commit will require us to have an inline wrapper for ::bind to use throughout the code; the current preprocessor-based approach breaks std::bind used in splay.cc:135 But I can't imagine a way to redefine ::bind that doesn't involve the preprocessor

[kinkie]

Re: merging: @yadij already approved so in theory it could be merged now; since there have been changed since I'd like to have @rousskov 's opinion

[rousskov]

since there have been changed since I'd like to have @rousskov 's opinion

My opinions have not changed.

[rousskov]

This work ... doesn't regress the general status and is a step forward to the intended goal.

I hope that is true, but I cannot validate that claim.

The XXX ... marked in the last commit will require us to have a ... wrapper for ::bind to use throughout the code; the current preprocessor-based approach breaks std::bind used in splay.cc:135. But I can't imagine a way to redefine ::bind that doesn't involve the preprocessor

What exactly prevents our Windows-specific wrapper from exporting a ::bind() function (rather than a macro)? The comments say "Each of these functions is defined in the Squid namespace so as not to clash with the winsock.h and winsock2.h definitions". AFAICT, those definitions use HANDLE as the type of the first argument. The definitions we add use int. Perhaps we can limit support to Windows flavors where HANDLE is not an integer and use polymorphism to provide our own versions of those functions where the first argument is an int?

If polymorphism does not work, we can try wrapping winsock.h and winsock2.h #include statements with macros that force Windows headers to declare windows_bind() instead of bind(). However, that is a huge and ugly hack, even if it works.

[kinkie]

This work ... doesn't regress the general status and is a step forward to the intended goal.

I hope that is true, but I cannot independently validate that claim.

The XXX ... marked in the last commit will require us to have a ... wrapper for ::bind to use throughout the code; the current preprocessor-based approach breaks std::bind used in splay.cc:135. But I can't imagine a way to redefine ::bind that doesn't involve the preprocessor

What exactly prevents our Windows-specific wrapper from exporting a ::bind() function (rather than a macro)? The comments say "Each of these functions is defined in the Squid namespace so as not to clash with the winsock.h and winsock2.h definitions". AFAICT, those definitions use HANDLE as the type of the first argument. The definitions we add use int. Perhaps we can limit support to Windows flavors where HANDLE is not an integer and use polymorphism to provide our own versions of those functions where the first argument is an int?

the callsites use bind(int..). The macro in compat/os/mswindows.h is for them to maintain the illusion that this is what the OS provides. That macro then calls Squid::bind(int..) which does the HANDLE lookup, calls ::bind(HANDLE), and adapts the error back.

Since the macro bind breaks std::bind, I am afraid that the next step is to define a Squid::bind which in the non-mswindows case just forwards the call to ::bind, and adapt callsites to use it. It might make sense to wrap all the network API calls in that case for cleanliness, and stop using the preprocessor for this compatibility shim

[rousskov]

the callsites use bind(int..). The macro in compat/os/mswindows.h is for them to maintain the illusion that this is what the OS provides. That macro then calls Squid::bind(int..) which does the HANDLE lookup, calls ::bind(HANDLE), and adapts the error back.

Please (re)interpret what I said, including my question, with the assumption that I know all of the above facts.

[rousskov]

In other words, what will break if we remove our macros and remove our Squid namespace?

[kinkie]

In other words, what will break if we remove our macros and remove our Squid namespace?

it builds. It might be that mingw is doing the emulation job for us? I can try to remove most of mswindows.h if that is the case and see what happens

[rousskov]

It might be that mingw is doing the emulation job for us? I can try to remove most of mswindows.h if that is the case and see what happens

I do not know the answer to the above question. I can only keep whining that we should understand what happens (instead of using trial-and-error approach to find the first magical combination that appears to work in a given black-box test). When it comes to Windows code, I can spot high-level problems and make suggestions based on basic principles, but I cannot do the legwork necessary to find, validate, and defend the exact Windows-specific solution. These problems only reinforce my doubts in the Project current abilities when it comes to providing Windows support, for any reasonable definition of "support"...

P.S. To avoid misunderstanding, by "remove Squid namespace", I meant removing namespace Squid { and the matching } characters while still keeping our custom wrappers defined inside those curly braces.

[kinkie]

Scope creep is too big, parts of this have been reworked in more constrained PRs. Abandoning