[CAGE] revisit t/tools/ops2pm/05-renum_op_map_file.t

[allison]

Just before the release, rurban pointed out that tools/dev/opsrenumber.pl had a feature to completely change its functionality when the major version changes to "1". The problem being that this functionality is entirely unused so far, poorly tested, and not actually what we want. I changed it in r37525 so it has no special behavior for different version numbers.

4 tests in t/tools/ops2pm/05-renum_op_map_file.t fail as a result, so I'm marking them as TODO. These tests need to be updated to reflect our expectations for op renumbering in 1.0 and beyond.

[jkeenan]

Replying to allison:

Just before the release, rurban pointed out that tools/dev/opsrenumber.pl had a feature to completely change its functionality when the major version changes to "1". The problem being that this functionality is entirely unused so far, poorly tested, and not actually what we want. I changed it in r37525 so it has no special behavior for different version numbers. 4 tests in t/tools/ops2pm/05-renum_op_map_file.t fail as a result, so I'm marking them as TODO. These tests need to be updated to reflect our expectations for op renumbering in 1.0 and beyond.

tools/dev/opsrenumber.pl started out its life as a Python program. Coke filed  RT 53976 nearly a 1 year ago to have it rewritten in Perl 5, which I did . Said rewrite is what is being tested in t/tools/ops2pm/05-renum_op_map_file.t. But I reached the limits of my understanding of the issues involved and, IIRC, Coke didn't consider that ticket closable. So it was assigned to Nobody.

Here is a far-from-exhaustive list of questions that arise at this time:

1. Where do we document what 'our expectations for ops renumbering for 1.0 and beyond' currently are?

2. If 1.0 should still be considered the major turning point, is it safe to ditch the code that was designed to handle pre-1.0?

Thank you very much.
kid51

[allison]

Your translation of the original Python program is perfect. The original just made some assumptions about post-1.0 behavior that we never exercised (since we'd always been pre-1.0).

On your questions:

1. The expectations for ops renumbering for 1.0 and beyond is exactly the same as pre-1.0. No additional documentation needed.

2. 1.0 isn't a turning point, it's a continuation of business as usual. We should ditch the code that was designed to handle post-1.0. In fact, I already ditched the code, so what's needed is to expand the tests to make sure the behavior pre- and post-1.0 is the same.

[jkeenan]

Thanks for the feedback. I hadn't noticed the March 17 change to lib/Parrot/Ops2pm/Base.pm. I will take the ticket and, in addition to correcting the test file, will review the documentation in tools/dev/opsrenumber.pl, which is probably out-of-date.

Thank you very much.
kid51

[jkeenan]

Replying to allison:

After many hours staring at tools/dev/opsrenumber.pl, lib/Parrot/OpsRenumber.pm, t/tools/ops2pm/05-renum_op_map_file.t and related files, I have become convinced that it does not do what we think it does -- or, at the very least, what I have long thought it did.

If you add ops codes, whether to an existing file or to a new file, make opsrenumber -- which is just a call to tools/dev/opsrenumber.pl -- does not automatically add those to src/ops/ops.num and appropriately renumber the ops codes in that file. src/ops/ops.num, in fact, does not change at all.

If, however, you delete ops codes and run make opsrenumber, src/ops/ops.num changes appropriately to reflect the absence of the deleted ops.

In my attempt to get t/tools/ops2pm/05-renum_op_map_file.t DWIMming in this ticket, the Stage 2 test stanza reflects deletion of ops codes, while Stage 3 represents addition. The Stage 2 tests pass; the Stage 3 tests don't and are TODO-ed out.

For a long time, I thought the problem lay in my tests and my difficulty in understanding the underlying code in files such as lib/Parrot/OpsFile.pm and lib/Parrot/Op.pm. But last night, while staring at Parrot::OpsRenumber::renum_op_map_file() -- the method underlying all the ops renumbering code -- I came to realize that it simply had no functionality to add ops codes to src/ops/ops.num.

    for ( @{ $self->{ops}->{OPS} } ) {
        if ( defined $fixed{ $_->full_name } ) {
            $n = $fixed{ $_->full_name };
        }
        elsif ( $seen{ $_->full_name } ) {
            printf $OP "%-31s%4d\n", $_->full_name, ++$n;
        }
    }

The Parrot::OpsFile call which results in @{ $self->{ops}->{OPS} } does pick up newly added ops codes. But the if-elsif code thereafter accounts only for (a) the 8 fixed ops codes and (b) those ops codes already found in src/ops/ops.num. There's no code there which says, Those new ops codes which you didn't find in ops.num? Add them at the end of ops.num.

Apart from noting this in t/tools/ops2pm/05-renum_op_map_file.t, I conducted a couple of experiments outside the Test::More context. I did a fresh checkout, created a dummy src/ops/ver.ops file, edited the Makefile to add that to $OPS_FILES, and called make opsrenumber. src/ops/ops.num was unchanged; the last opcode was numbered 1248.

I then killed ver.ops and temporarily deleted the real ops file var.ops as well. I then edited the Makefile to reflect the deletion of var.ops from $OPD_FILES. In this case, src/ops/ops.num was changed: the last opcode was now numbered 1193.

I would infer from this that the only thing make opsrenumber is accomplishing with respect to addition of opscodes is that, when someone manually adds opscodes to the end of the file, running make opsrenumber assigns new numbers.

My questions are: Is my analysis correct? If so, then my long-held conception of what make opsrenumber was doing is wrong. But is that then what we want make opsrenumber or tools/dev/opsrenumber.pl to be doing? Wouldn't we really want it to automatically detect newly added ops codes and add them with appropriate numbers to src/ops/ops.num?

[rurban]

Replying to allison:

Your translation of the original Python program is perfect. The original just made some assumptions about post-1.0 behavior that we never exercised (since we'd always been pre-1.0). On your questions: 1. The expectations for ops renumbering for 1.0 and beyond is exactly the same as pre-1.0. No additional documentation needed. 2. 1.0 isn't a turning point, it's a continuation of business as usual. We should ditch the code that was designed to handle post-1.0. In fact, I already ditched the code, so what's needed is to expand the tests to make sure the behavior pre- and post-1.0 is the same.

In the contrary 1.0 was a huge turning point.

Allison removed with two single strokes all the pbc compatibility code and docs which existed for years. She not only disabled the platform portability tests, now also pbc cross-version portability is gone, contrary to the initial project goal, pre-1.0. This is the reason why I had to leave the project.

parrot does not care obviously, but the languages are hit deeply, as now the idea of language pbc's will not work across versions. A perl6.pbc or group.so will not help pipp and the other langs as it will be next to impossible to maintain coordinated releases of all langs together. So rakudo will survive, but the other languages are effectively dead.

[allison]

Jim,

It's not supposed to add new opcodes to ops.num. Those have to be manually added, and that's an intentional control step, so someone has to think about which ops are being added. But, new ops must always be added to the end of the file (unless you want to manually renumber all the ops that follow your insertion point, which would be insane). What opsrenumber.pl does is put them in a sane order. (For example, if you added a new form of the 'print' opcode that takes an extra argument, for long-term maintainability we don't really want it to numbered 1295, while all the other 'print' opcodes are numbered 300-315.)

I'm also removing the 1.2 milestone from this ticket, as it's not a target for any particular release.

[jkeenan]

Summation of changes applied in r38825 and r38829

[jkeenan]

Replying to allison:

Jim, Those have to be manually added, and that's an intentional control step, so someone has to think about which ops are being added. But, new ops must always be added to the end of the file (unless you want to manually renumber all the ops that follow your insertion point, which would be insane). What opsrenumber.pl does is put them in a sane order.

Thanks for the clarification. In r38825 and r38829, I applied the changes contained in the attached patch, opsrenum.patch.

The revised test file does not contain a test for the case where we have added opcodes and then added those opcodes to the tail of src/ops/ops.num. I tried to write such a test, but my head started spinning with all the manipulations I would have to do to get it to work correctly in a tempdir. So, instead, I did a fresh checkout from trunk and proceeded to manipulate src/ops/core.ops and src/ops/ops.num until I was persuaded that tools/dev/opsrenumber.pl DWIMmed.

So I think this fixes the issue, and I am therefore closing this ticket.

Thank you very much.

kid51