Commenting below ...
On 2/5/18 4:03 PM, David Hotham wrote:
> ... but otherwise most are pretty stale.
As submitter of one of those 'stale' pull requests - #96 - this is frustrating
to read.
I commented on #96 myself (I'm bradh352) and that was the last it was touched it
appears. I had said that I thought ares_const_channel would probably be the better
option short of the "right" solution which would be something like:
typedef struct ares_channeldata ares_channel_t;
Then change all public prototypes that take "ares_channel channel" to instead take
"ares_channel_t *channel".
( Since obviously the new "const ares_channel_t *channel" != "const ares_channel
channel", and you specifically
want the proposed semantics )
If we left the current ares_channel typedef in place and switched to
ares_channel_t for the one that doesn't hide
the pointer for all public prototypes, in theory we would maintain full ABI
compatibility and partial API compatibility
(partial as some compilers may emit warnings, but others may not since they
really are equivalent).
Personally I'd view it as worthwhile for long-term project viability, but
others may not agree, and I'm currently not
senior enough to make those kind of decisions.
Understood that we all are busy people, but still: in the case of my pull
request and several others, staleness is induced only by maintainer
inactivity. So it is, as I say, frustrating to see it suggested that this is
itself a good reason for continuing to allow such PRs to languish.
As I wrote in a similar thread before a previous release: in the particular
case of #96 I wouldn't be overly upset to see it rejected - and I would prefer
that over leaving it dangling indefinitely.
Its sometimes hard to get other devs to review things like this, especially
when you're talking about changing the existing public API that's been that way
for years.
Some of the open pull requests are indeed complex and I recognise that they'll
require real thought to be dealt with properly. It's a shame that there isn't
enough maintainer resource to deal with this - perhaps the project should be
looking for new co-maintainers? - but such is life.
A couple of new people have been added fairly recently like myself, but of
course, we tend to have day jobs and work on our own needs (or needs of the
company we work for) first :/
However several PRs look as though they really could be dealt with very cheaply
at this point. I suggest:
* #47 just wants accepting: it's obviously harmless, someone was bothered
enough to submit a fix, let's do them the favour of taking it. (It's
particularly sad that this one gets to be two years old).
Well, the PR technically needs to be updated as it doesn't solve the issue any more. There are 3 calls now days to GetVersionEx() in c-ares, so the other 2 will still emit warnings. This is probably one of the reasons it wasn't accepted when I reviewed pending PRs. The next reason is the reason
for the warning is Microsoft wants you to use VerifyVersionInfo() these days as they say GetVersionEx() may not be available or return the right thing in the future, so that would be the more proper fix, but would involve killing ancient Windows versions (which I'd be fine with, but can't guarantee
others may not have a different opinion).
* #57 just wants rejecting, per the comments explaining that it's unnecessary
I left this open as a reminder to myself to update the manpage as the *real*
fix for this, I'll close it when I do it. Probably a good thing to do with
the next release cycle.
* #158 looks like it wants rejecting, per the comments in #157
Correct, again same as #57, left this open as a reminder to myself to clarify
in the manpage that it is the caller's responsibility to enable anything
necessary for networking calls in the OS (such as Windows WSAStartup).
* #170 just wants accepting: it's another that's obviously harmless. Leaving
this sort of thing dangling can only discourage contributors.
I'm new so I'm not familiar with the c-ares copyright policy stuff, I know
there's also dates in most of the source files and headers that are out of date
so I personally think the pull request doesn't address enough to be complete.
That lot (and #96) would be enough to close a third of the open pull requests.Â
Isn't this worthwhile?
I can definitely do #57 and #158 myself, the rest I'll need feedback from other
devs on.
On 05/02/2018 14:44, Brad House wrote:
It seems like there's been a lot of fixes for Windows and Android since our last official release. I think it might be a good idea to start release planning for another release. Any thoughts on this or any PRs that should be addressed first? I think it might be good to see gjasny finish up
PR#168 for the release, but otherwise most are pretty stale.
-Brad
-Brad
