re: [PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW
Hi Frédéric, your patch series seems to have failed the cmd tests: batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301 (got 'Syntax error', wanted '1') batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302 (got '1', wanted '3') batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303 (got '3', wanted '5') See http://buildbot.kegel.com/builders/runtests/builds/86 I'll change the buildbot so it tests each subset of a patch series, and pinpoint the one that broke things. Incidentally, that 'Syntax error' probably should go to stderr; and since the cmd tests ignore stderr, that might fix your failure.
2011/8/19 Dan Kegel <dank(a)kegel.com>:
Hi Frédéric, your patch series seems to have failed the cmd tests:
batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301 (got 'Syntax error', wanted '1') batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302 (got '1', wanted '3') batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303 (got '3', wanted '5')
See http://buildbot.kegel.com/builders/runtests/builds/86
I'll change the buildbot so it tests each subset of a patch series, and pinpoint the one that broke things.
Incidentally, that 'Syntax error' probably should go to stderr; and since the cmd tests ignore stderr, that might fix your failure.
They should eventually go to stderr, but I wouldn't change them right now. The current tests can run successfully only if the # lines match in the output / expected files, so putting them to stderr instead might/will break things. Also, they are (only?) generated for perfectly valid constructs due to parsing bugs in current code. I don't know how your code works, and it's probably done already, but in certain cases if tests fail, some test files/dirs can be left in the test directory, so a preliminary cleanup is probably a wise idea. Frédéric
2011/8/19 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
Incidentally, that 'Syntax error' probably should go to stderr; and since the cmd tests ignore stderr, that might fix your failure.
They should eventually go to stderr, but I wouldn't change them right now. The current tests can run successfully only if the # lines match in the output / expected files, so putting them to stderr instead might/will break things.
Do our tests pass on windows cmd? That outputs errors to stderr, I think.
I don't know how your code works, and it's probably done already, but in certain cases if tests fail, some test files/dirs can be left in the test directory, so a preliminary cleanup is probably a wise idea.
The buildbot always does clean builds/tests. - Dan
2011/8/19 Dan Kegel <dank(a)kegel.com>:
Hi Frédéric, your patch series seems to have failed the cmd tests:
batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301 (got 'Syntax error', wanted '1') batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302 (got '1', wanted '3') batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303 (got '3', wanted '5')
See http://buildbot.kegel.com/builders/runtests/builds/86
I'll change the buildbot so it tests each subset of a patch series, and pinpoint the one that broke things.
The last [10/10] patch was the cause. I sent an updated version. I wonder how/whether the various (resend) or (try N) are handled for revised patch series Frédéric
2011/8/19 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
The last [10/10] patch was the cause. I sent an updated version.
Hmm. You sent 10/10 before 1/10. My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order".
I wonder how/whether the various (resend) or (try N) are handled for revised patch series
My recognizer does not notice those designations, nor the time or order of arrival. It only looks at the 'Date:' and 'From:' fields, and the '[%d/%d]' part of the subject line. (The exact regular expression I use is \[\D*(\d+)/(\d+)\D*\] .) Humans prefer (resend) for unchanged resends, and (try N+1) for changed resends, I think. - Dan
2011/8/20 Dan Kegel <dank(a)kegel.com>:
2011/8/19 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
The last [10/10] patch was the cause. I sent an updated version.
Hmm. You sent 10/10 before 1/10.
Yeah, I thought it was sufficient, but testbot choked on it and waited for the remaining patches in the series.
My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order".
I wonder how/whether the various (resend) or (try N) are handled for revised patch series
My recognizer does not notice those designations, nor the time or order of arrival. It only looks at the 'Date:' and 'From:' fields, and the '[%d/%d]' part of the subject line. (The exact regular expression I use is \[\D*(\d+)/(\d+)\D*\] .)
Using the dates of the git format-patches can be problematic, e.g. when you local git branch has been modified before submission (think rebase -i)
Humans prefer (resend) for unchanged resends, and (try N+1) for changed resends, I think.
As I said, I have sent an updated/fixed version of the [10/10]. Then (for the unchanged one) [1/10] <...> (resend) (try 2) ... [9/10] <...> (resend) (try 2) once I saw testbot was waiting for the rest of the series, which it processed afterwards IIRC someone told me some time ago to resent the whole series with (try N+1) for tests processed by testbot. The (resend) is just a hint to AJ that that very patch hasn't been revised, just regenerated. IMHO your patch series recognizer should use the same strategy as testbot, for consistency, and wait for the whole series to be available before processing can start. Frédéric
2011/8/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
IIRC someone told me some time ago to resent the whole series with (try N+1) for tests processed by testbot. The (resend) is just a hint to AJ that that very patch hasn't been revised, just regenerated.
Yeah. And sometimes the body says "No changes since last try", in which case maybe even the (resend) hint isn't needed.
IMHO your patch series recognizer should use the same strategy as testbot, for consistency
I see his code at http://wine.git.sourceforge.net/git/gitweb.cgi?p=wine/winetestbot;a=blob;f=l... http://wine.git.sourceforge.net/git/gitweb.cgi?p=wine/winetestbot;a=blob;f=l... and can see a few details, but his overall strategy is hard to discern in a quick read.
and wait for the whole series to be available before processing can start.
That I do. - Dan
2011/8/20 Dan Kegel <dank(a)kegel.com>:
2011/8/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
IIRC someone told me some time ago to resent the whole series with (try N+1) for tests processed by testbot. The (resend) is just a hint to AJ that that very patch hasn't been revised, just regenerated.
Yeah. And sometimes the body says "No changes since last try", in which case maybe even the (resend) hint isn't needed.
Probably more straightforward to use the (resend) in the subject, so you don't have to check the message contents
On 08/20/2011 05:06 AM, Dan Kegel wrote:
2011/8/19 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
The last [10/10] patch was the cause. I sent an updated version.
Hmm. You sent 10/10 before 1/10. My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order". You cannot rely on that. git send-email of a patch series pushes the emails very fast out and with MX server clusters you run into race conditions.
bye michael
On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 08/20/2011 05:06 AM, Dan Kegel wrote:
Hmm. You sent 10/10 before 1/10. My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order". You cannot rely on that. git send-email of a patch series pushes the emails very fast out and with MX server clusters you run into race conditions.
I thought the Date: field was set by the client, so server races shouldn't matter. If I run into messages with identical dates, I could sort first by date, then by patch number within the series. - Dan
On 08/20/2011 04:28 PM, Dan Kegel wrote:
On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 08/20/2011 05:06 AM, Dan Kegel wrote:
Hmm. You sent 10/10 before 1/10. My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order". You cannot rely on that. git send-email of a patch series pushes the emails very fast out and with MX server clusters you run into race conditions.
I thought the Date: field was set by the client, so server races shouldn't matter. Sorry, missed that you use that and not the order you receive it.
If I run into messages with identical dates, I could sort first by date, then by patch number within the series.
bye michael
On Sat, Aug 20, 2011 at 16:34, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 08/20/2011 04:28 PM, Dan Kegel wrote:
On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 08/20/2011 05:06 AM, Dan Kegel wrote:
Hmm. You sent 10/10 before 1/10. My patch series recognizer rejected the series. It would be hard to fix. Let's see if we can live with the rule "patch series must be sent in order". You cannot rely on that. git send-email of a patch series pushes the emails very fast out and with MX server clusters you run into race conditions.
I thought the Date: field was set by the client, so server races shouldn't matter. Sorry, missed that you use that and not the order you receive it.
That's not always reliable: say you commit locally patches [1-2/3] on day D and patch [3/3] on day D+1 P3 - baz - D+1 P2 - bar - D P1 - foo - D For whatever reason, before submission, you decide patch [3/3] should be the first one, so you use "git rebase" do to that: P3 - bar - D P2 - foo - D P1 - baz - D+1 and you git-send the mails. So, you should always use the numbering specified by the author IMHO
If I run into messages with identical dates, I could sort first by date, then by patch number within the series.
Frédéric
2011/8/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
I thought the Date: field was set by the client, so server races shouldn't matter. Sorry, missed that you use that and not the order you receive it.
That's not always reliable: say you commit locally patches [1-2/3] on day D and patch [3/3] on day D+1 P3 - baz - D+1 P2 - bar - D P1 - foo - D
For whatever reason, before submission, you decide patch [3/3] should be the first one, so you use "git rebase" do to that: P3 - bar - D P2 - foo - D P1 - baz - D+1
and you git-send the mails.
git-send's messages' timestamps use current time, not commit time: http://git.kernel.org/?p=git/git.git;a=blob;f=git-send-email.perl#l98 And it is careful to increment the time after each message! So I think my usage is compatible with git-send.
So, you should always use the numbering specified by the author IMHO
I wish it were so easy. It is very difficult to recognize patch series without relying on them being sent in order. I've tried: take all unprocessed messages divide them by sender further divide them into groups based on length of patch series in subject line sort each group by patch number from subject line but that breaks down if the developer sends two patch series with the same length, which happens very often, e.g. when somebody retries a patch series. Maybe one could further subdivide messages by retry number, but that sounds hard and fragile. - Dan
2011/8/20 Dan Kegel <dank(a)kegel.com>:
2011/8/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
So, you should always use the numbering specified by the author IMHO
I wish it were so easy. It is very difficult to recognize patch series without relying on them being sent in order. I've tried: take all unprocessed messages divide them by sender further divide them into groups based on length of patch series in subject line sort each group by patch number from subject line but that breaks down if the developer sends two patch series with the same length, which happens very often, e.g. when somebody retries a patch series. Maybe one could further subdivide messages by retry number, but that sounds hard and fragile.
Hmm well we/AJ could institute some rules for resending patch series, so that the whole series is resubmitted with the same (try N) "marker", even when only one patch is changed, possibly preceded by a (resend) [i.e. the subdivision by retry number you're talking about], and document that in the wiki. That would probably help a bit, but in that case, there should be a filter on wine-patches/testbot so it refuses ill-formed messages (like with (resen*t*), etc.) in order to enforce that rule, e.g. by checking the subject line with a regexp like ^.*( resend)?( try \d+)?$ Just my 2 ¢ Frédéric
2011/8/19 Dan Kegel <dank(a)kegel.com>:
I'll change the buildbot so it tests each subset of a patch series, and pinpoint the one that broke things.
For the record, this is implemented now, e.g. http://buildbot.kegel.com/builders/runtests/builds/99 is 9/10, and is ok http://buildbot.kegel.com/builders/runtests/builds/100 is 10/10, and fails And it was handy already: http://buildbot.kegel.com/builders/runtests/builds/103 was 1/3, and was ok http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and had problems.
2011/8/20 Dan Kegel <dank(a)kegel.com>:
2011/8/19 Dan Kegel <dank(a)kegel.com>:
I'll change the buildbot so it tests each subset of a patch series, and pinpoint the one that broke things.
For the record, this is implemented now, e.g. http://buildbot.kegel.com/builders/runtests/builds/99 is 9/10, and is ok http://buildbot.kegel.com/builders/runtests/builds/100 is 10/10, and fails
And it was handy already: http://buildbot.kegel.com/builders/runtests/builds/103 was 1/3, and was ok http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and had problems.
I like the "blamelist" section ;) But seriously you could probably remove it altogether since the email address is already in the "Reason" (unless I misunderstand its function) Frédéric NB: the time to process each patch seems rather high (14 mins). Maybe you could use ccache to speed up things
2011/8/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
And it was handy already: http://buildbot.kegel.com/builders/runtests/builds/103 was 1/3, and was ok http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and had problems.
I like the "blamelist" section ;) But seriously you could probably remove it altogether since the email address is already in the "Reason" (unless I misunderstand its function)
I would have to change buildbot's code to make it only show up in one of those places, I think.
NB: the time to process each patch seems rather high (14 mins). Maybe you could use ccache to speed up things
True, I should try that. And patch series could be sped up even more by doing incremental builds for the 2nd, 3rd, etc. patches in the series. I haven't looked into how easy/hard that would be. - Dan
participants (3)
-
Dan Kegel -
Frédéric Delanoy -
Michael Stefaniuc