Re: cmd: Adjust CMD_LIST to a tree-like structure and implement command chains
Hi, Some time ago I was told that every patch in a patch set all patches should produce a state that passes all the tests. Given how much the current code is interconnected, i don't think i can split this while keeping the tests functional. How should I proceed in this case, can I split the code into logical bits knowing that the intermediate states will not pass the tests? Flávio J. Saraiva 2017-03-21 21:06 GMT+00:00 Marvin <testbot(a)winehq.org>:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Needs splitting".
This means that the patch needs to be split, either because it contains unrelated changes that should be their own separate patch, or because it's too large for review. You need to find a way to resend the changes in smaller chunks.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
On Wed, Mar 22, 2017 at 11:34 AM, Flávio J. Saraiva <flaviojs2005(a)gmail.com> wrote:
Hi,
Some time ago I was told that every patch in a patch set all patches should produce a state that passes all the tests. Given how much the current code is interconnected, i don't think i can split this while keeping the tests functional.
How should I proceed in this case, can I split the code into logical bits knowing that the intermediate states will not pass the tests?
Flávio J. Saraiva
You could start by submitting test-only patches first, demonstrating the issues. Then, split the code. For instance, your patch says "Adjust CMD_LIST to a tree-like structure and implement command chainsAdjust CMD_LIST to a tree-like structure and implement command chains", maybe the structure change could be a single patch, ensuring tests still pass? Then add one or more patches fixing the tests. Just my 2 ¢ -- Frédéric Delanoy
2017-03-21 21:06 GMT+00:00 Marvin <testbot(a)winehq.org>:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Needs splitting".
This means that the patch needs to be split, either because it contains unrelated changes that should be their own separate patch, or because it's too large for review. You need to find a way to resend the changes in smaller chunks.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
I wish it were that simple... Here's the current situation: 1) some months ago I already added tests that demonstrate the command chain problems (which are most of the @todo_wine@ that i'm removing in the commit because they were fixed) 2) IF and FOR actually contain command chains: "IF [«options»] «test» «chain» [ELSE «chain»]" "FOR [«options»] «var» IN («stuff») DO «chain»" 3) the current tests are heavily dependent on IF, FOR and CALL 4) currently IF, ELSE and FOR are parsed in a hackish way (causing the first command to be part of the "if ", "else " or "do " command unless it starts with parens), and because of that particular parsing problem the IF and FOR commands don't follow the normal execution path... for some reason parsing was never fixed and instead we get WCMD_part_execute, which is only needed because of the hackish parsing 5) CALL is also handled in a hackish way that just screams "spaghetti code" (in particular the called/retryCall parameter and all it's hard to grasp side effects) 6) pipes and redirects are also handled in a hackish way that only works for simple cases 7) labels are pretty simple, I just have to make sure they aren't executed In short: - if I try to fix IF or FOR first then i'm forced to add chains to avoid getting lots of errors - if I try to add chains first then i'm forced to fix IF and FOR to avoid getting lots of errors - if I change FOR then I have to handle the difficult 'CALL FOR' test, which relies on a spagetti of hackish code So I ask again, considering that I don't see how it can be split while keeping the tests working, how should I proceed? Is it ok if I split it into logical bits and only have the tests working at the end? Maybe splitting the final test file modifications is enough? Flávio J. Saraiva 2017-03-23 17:14 GMT+00:00 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
On Wed, Mar 22, 2017 at 11:34 AM, Flávio J. Saraiva <flaviojs2005(a)gmail.com> wrote:
Hi,
Some time ago I was told that every patch in a patch set all patches should produce a state that passes all the tests. Given how much the current code is interconnected, i don't think i can split this while keeping the tests functional.
How should I proceed in this case, can I split the code into logical bits knowing that the intermediate states will not pass the tests?
Flávio J. Saraiva
You could start by submitting test-only patches first, demonstrating the issues. Then, split the code. For instance, your patch says "Adjust CMD_LIST to a tree-like structure and implement command chainsAdjust CMD_LIST to a tree-like structure and implement command chains", maybe the structure change could be a single patch, ensuring tests still pass? Then add one or more patches fixing the tests.
Just my 2 ¢
-- Frédéric Delanoy
2017-03-21 21:06 GMT+00:00 Marvin <testbot(a)winehq.org>:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Needs splitting".
This means that the patch needs to be split, either because it contains unrelated changes that should be their own separate patch, or because it's too large for review. You need to find a way to resend the changes in smaller chunks.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
2017-03-22 11:34 GMT+01:00 Flávio J. Saraiva <flaviojs2005(a)gmail.com>:
Hi,
Some time ago I was told that every patch in a patch set all patches should produce a state that passes all the tests. Given how much the current code is interconnected, i don't think i can split this while keeping the tests functional.
How should I proceed in this case, can I split the code into logical bits knowing that the intermediate states will not pass the tests?
The tests should always pass. Notice though that you can mark the tests failing on Wine (temporarily or not) as "todo_wine" so that the failure is actually expected and not reported as such.
On 24 March 2017 at 00:10, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
The tests should always pass. Notice though that you can mark the tests failing on Wine (temporarily or not) as "todo_wine" so that the failure is actually expected and not reported as such.
You can, but that still potentially regresses functionality for applications, which is what we ultimately care about. It may be justified in some situations, but if at all possible it's still best avoided.
2017-03-24 0:15 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 24 March 2017 at 00:10, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
The tests should always pass. Notice though that you can mark the tests failing on Wine (temporarily or not) as "todo_wine" so that the failure is actually expected and not reported as such.
You can, but that still potentially regresses functionality for applications, which is what we ultimately care about. It may be justified in some situations, but if at all possible it's still best avoided.
If there is some way to split the patch properly while avoiding regressions that's certainly best. I guess I assumed that there is no clear way to do that and the tests temporarily breaking are somewhat edge cases (i.e. the functionality loss isn't really an issue in practice). If that's not the case then sure, todo_wines would be "cheating"...
2017-03-23 23:15 GMT+00:00 Henri Verbeet <hverbeet(a)gmail.com>:
You can, but that still potentially regresses functionality for applications, which is what we ultimately care about. It may be justified in some situations, but if at all possible it's still best avoided.
If what winehq untimately cares about is not having regressions, then the best approach here is to split it into logical bits (only passing tests at the end of the patch set) and not try to minimize the changes in the parts i'm being forced to change. This means that if one patch gets rejected then the whole set is also rejected. I guess this wouldn't be a problem? I'll proceed like this for now and review this slowly. In the current patch the only part that might contain an untested regression is the 'CALL FOR' part (that's why it has a FIXME). Flávio J. Saraiva
Am 24.03.2017 um 17:26 schrieb Flávio J. Saraiva <flaviojs2005(a)gmail.com>:
If what winehq untimately cares about is not having regressions, then the best approach here is to split it into logical bits (only passing tests at the end of the patch set) and not try to minimize the changes in the parts i'm being forced to change.
This means that if one patch gets rejected then the whole set is also rejected. I guess this wouldn't be a problem?
It is a potential problem for bisects. Let's assume I am bisecting a direct3d regression in an app that also uses cmd.exe. Somewhere between the good and bad revisions for the d3d problem there is a revision where the app fails because of cmd. That said, this sometimes happens and it's solvable with some extra work during bisects (e.g. manually applying the fix for cmd for revisions that are broken in a way I don't care). But this is one of the reasons why e.g. having a temporary build failure is a no-go.
The patches will always build, the tests are what will fail (will need the full patch set). To avoid the temporary failure of tests the patch set needs to be applied in one go (commit to a separate branch and then merge to master?), but that's out of my hands. I got "Needs splitting" so that's what i'll do, and this time I won't try to minimize changes to other code (and therefore avoid adding hacks). Flávio J. Saraiva 2017-03-25 10:58 GMT+00:00 Stefan Dösinger <stefandoesinger(a)gmail.com>:
Am 24.03.2017 um 17:26 schrieb Flávio J. Saraiva <flaviojs2005(a)gmail.com>:
If what winehq untimately cares about is not having regressions, then the best approach here is to split it into logical bits (only passing tests at the end of the patch set) and not try to minimize the changes in the parts i'm being forced to change.
This means that if one patch gets rejected then the whole set is also rejected. I guess this wouldn't be a problem?
It is a potential problem for bisects. Let's assume I am bisecting a direct3d regression in an app that also uses cmd.exe. Somewhere between the good and bad revisions for the d3d problem there is a revision where the app fails because of cmd.
That said, this sometimes happens and it's solvable with some extra work during bisects (e.g. manually applying the fix for cmd for revisions that are broken in a way I don't care). But this is one of the reasons why e.g. having a temporary build failure is a no-go.
participants (5)
-
Flávio J. Saraiva -
Frédéric Delanoy -
Henri Verbeet -
Matteo Bruni -
Stefan Dösinger