PDP-10 / its

Incompatible Timesharing System
Other
841 stars 80 forks source link

Fixed issue with VERB? in Zork when issued following an error in parsing input line. #2178

Closed eswenson1 closed 10 months ago

eswenson1 commented 1 year ago

Fixes NTH OUT-OF-BOUNDS error when issuing a command like tell master "kill me with sword" in endgame when either there is no master there or no sword there.

larsbrinkhoff commented 1 year ago

I can't read this but I'm assuming it's fine.

The KA10 build failed twice. I'm restarting and hoping it was a fluke. (Software development best practice.)

larsbrinkhoff commented 1 year ago

Of of four runs on pdp10-ka, three timed out. Checking the last failure I see this:

*:xxfile lcf;comp log_lcf;comp xxfile
:PROCED
*
Job XXFILE interrupted: .VALUE;
$p
4247>>.VALUE 6657    $P

:KILL 
*:xxfile lcf;zork log_lcf;zork xxfile
:PROCED
*

The last command timed out.

That .VALUE is a expected part of the script, so apparently the second :xxfile takes too long now?

eswenson1 commented 1 year ago

Based on others’ having issues with the muddle compiles timing out on slow machines, we may wish to bump up the default timeout before compiling Zork. And from the above, we should bump it up for the load and save of Zork too. That one is a bit surprising though because it goes pretty fast on my machine — especially compared to the compiles.

It would be interesting to look at the LCF;Zork log file…

Yesterday, on ES, I had that second XXFILE hang on me due to a “job changed” error from XXFILE. I moved the appropriate control code down a line and it worked. Not sure why it didn’t fail all the time previously.

larsbrinkhoff commented 1 year ago

I increased the timeout for the second xxfile.

eswenson1 commented 1 year ago

I'm wondering, though, whether it isn't a timeout issue with the second XXFILE. That one shouldn't take very long. I'm wondering if the Control-^ R command that is at the top of the LCF;ZORK XXFILE file should move down a line -- under the ":mudsav;mud55" command line.

I had to make that change on ES yesterday in order to get it to run. Otherwise, it would (when outputting to TTY:) say that the job change wasn't expected and simply hang forever.

larsbrinkhoff commented 1 year ago

Ok, so maybe roll back the extended timeout and try the ^^ R instead.

But if that's what #2179 is doing, it didn't work very well.

eswenson1 commented 1 year ago

I’ve some real flakiness with XXFILE (and this one in particular) sometimes the ^^R seems to be required, and sometimes only removing it seems to allow it to work. Sometimes if needed, if has to be before the first command and other times directly after it.

Yesterday, my pdp10-ks build timed out on this command and re-executing it manually, showed that the first MDL expression evaluated was actually sent to DDT (with bad results and a timeout). I removed the ^^R entirely and it completed fine.

eswenson1 commented 1 year ago

@larsbrinkhoff what's with this PR? Why can't we merge? What should we do to get it so that it is mergeable?

larsbrinkhoff commented 1 year ago

I'm reading the comments and it seems you had some reservation: https://github.com/PDP-10/its/pull/2178#issuecomment-1481713211

The zork build script seems to work pretty well on the master branch. Do we need to do anything about ^^R or timeouts?

eswenson1 commented 1 year ago

To be honest, I don't know. I've built from the master branch several times now with no errors (of the ITS part), so perhaps this should just sit here until someone claims it is "better" than the master branch in some ways.

larsbrinkhoff commented 1 year ago

Yes. Maybe close the pull request for now? It can be reopened later, or a new pull request can be made.

larsbrinkhoff commented 10 months ago

Closing per above.