Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement makeWin98Image #9

Merged
merged 1 commit into from
Dec 30, 2023
Merged

Conversation

io12
Copy link
Contributor

@io12 io12 commented Dec 16, 2023

I tested this only on x86_64-linux. Sorry if some things weren't done the best way.

@MatthewCroughan
Copy link
Owner

You should add a repeatability test, since it will prove it works 100 times like the rest, I'm pretty impressed though.

@io12
Copy link
Contributor Author

io12 commented Dec 17, 2023

You should add a repeatability test, since it will prove it works 100 times like the rest, I'm pretty impressed though.

Done. Do I need to run it myself or does the Hercules CI do it?

@MatthewCroughan
Copy link
Owner

MatthewCroughan commented Dec 17, 2023

@io12 I have to bring your PR into the repo as a branch, which I just did, now the CI will start running.

https://hercules-ci.com/github/MatthewCroughan/NixThePlanet/jobs/351

@io12
Copy link
Contributor Author

io12 commented Dec 17, 2023

It seems to not be repeatable unfortunately due to keypresses dropping. I'll try to fix it.

@MatthewCroughan
Copy link
Owner

@io12 Yeah, one of the todos is perhaps to try xdotool to get the keypresses to stop dropping, I've found vncdo to be terrible.

@io12 io12 force-pushed the win98 branch 2 times, most recently from 4a93463 to 898901e Compare December 18, 2023 06:08
@io12
Copy link
Contributor Author

io12 commented Dec 18, 2023

Okay, I think it's more reliable now. Can you pull from the branch to run the CI again?

@MatthewCroughan
Copy link
Owner

@MatthewCroughan
Copy link
Owner

@io12 It appears that 10 of the instances have gotten stuck at the following step

image

Whereas two others are stuck at this without continuing
image

@MatthewCroughan
Copy link
Owner

MatthewCroughan commented Dec 18, 2023

Windows/msdos releases in the repo that I've tested, only 1 in 1000 would fail due to what seems like runtime bugs with dosbox, which is fine. But the other failed states can be worked around. I'm happy with 1 in 1000 failure rate, maybe even 1 in 100 is good enough for the purposes of the repo.

@io12
Copy link
Contributor Author

io12 commented Dec 18, 2023

Kind of frustrating that it's stuck at the final step 🙃. I'm not sure whether it's not fully booted yet and isn't ready to open the start menu or some other issue. Also, how did you get the screenshots?

@MatthewCroughan
Copy link
Owner

Nix does its work in /tmp, so I just go in there and extract the screenshots, same way as the macos repeatability test defined in this repo works.

@io12
Copy link
Contributor Author

io12 commented Dec 21, 2023

I made more repeatability improvements. Can you run the CI again?

@MatthewCroughan
Copy link
Owner

@io12 some fail like this, maybe dosbox's fault?

image

Some seem to still get stuck at the end like this

image

@MatthewCroughan
Copy link
Owner

@io12 Statistically, your change did have an impact, as now at least 68 in 100 attempts work, as opposed to the previous 22.

@MatthewCroughan
Copy link
Owner

Maybe we could just kill -9 the process when it gets to the start menu instead of trying to shut down properly?

@io12
Copy link
Contributor Author

io12 commented Dec 22, 2023

Maybe we could just kill -9 the process when it gets to the start menu instead of trying to shut down properly?

That would work but then there's a ScanDisk improper shutdown message on the next boot. I think the issue is sometimes when the start menu is open before the "welcome to windows" window, the start menu can close. I think maybe the failing cases try to select the shutdown option after this happens. I'm trying to fix this by repeating the ctrl-esc u enter combination until the desktop icons and start menu disappear.

@MatthewCroughan
Copy link
Owner

@io12 rarely, this failure happens

image

@io12
Copy link
Contributor Author

io12 commented Dec 23, 2023

Okay, I think I fixed both issues and it repeats locally 16 times. Hopefully it works all 100 times now.

@MatthewCroughan
Copy link
Owner

@io12

Wow, the failure rate now seems to be 3 in 100.

The three that have failed, have failed in a strange way though, as they have gotten to the ### OMG DID IT WORK???!!!! ### log line. Here are their logs for you to inspect.

There are the last three screenshots that were taken by the expect script, but the screnshots are no longer being taken by the process, as their last modified time is no longer updating periodically.

image
image
image

@io12
Copy link
Contributor Author

io12 commented Dec 24, 2023

Thanks! Such a rare issue is pretty tricky to debug, but I removed a code path left over from a different approach that triggers in all three and doesn't trigger normally. Even if it doesn't fix the issue it should at least give more up-to-date screenshots.

@io12
Copy link
Contributor Author

io12 commented Dec 24, 2023

Also, I wonder how hard it would be to patch dosbox-x to fix the non-repeatability issues. If the issue comes from the timing of keypresses, maybe we can make dosbox-x take an input file that automatically triggers keypresses at specified cycle counts? I'm not sure if this would actually make things fully deterministic, but maybe worth a shot? And I don't think this is the first time Nix packages have patched build tools to make them more deterministic?

@MatthewCroughan
Copy link
Owner

1 in 100 seems end up like this LOL!

image

Whereas there are one other stuck like this

image

@MatthewCroughan
Copy link
Owner

@io12 Have you got a matrix or discord we can talk more actively on?

Repository owner deleted a comment from io12 Dec 28, 2023
@MatthewCroughan MatthewCroughan merged commit e6c417c into MatthewCroughan:master Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants