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

Set up cicd, build SBEMU.EXE and package it in FreeDOS USB image #21

Merged
merged 62 commits into from
Dec 6, 2023

Conversation

volkertb
Copy link
Collaborator

@volkertb volkertb commented Jun 4, 2023

@crazii Here you go, a first working verison of a CI/CD pipeline for SBEMU. 😅

The idea being that each commit or merge to the main branch should trigger the build and release of a ready-to-use USB image that users and testers can immediately flash to a USB drive or SD card to immediately test SBEMU on their PCs, laptops or VMs.

A few things that still need to be addressed:

  • The right version/build of HDPMI32i.EXE still needs to be included, but I don't know where to obtain the correct version. Even better would be to have the correct version being built in a CI/CD pipeline and versioned as well.
    • Update 2023-12-03: Done. @crazii released an up-to-date build yesterday, and the branch has been updated to include it in the USB image and have it automatically loaded on start-up, as well as SBEMU itself and the CuteMouse mouse driver.
  • I had to write a manual build script (build.sh) since the Makefile that GPR2MAK generates from sbemu.gpr does not work. See GitHub issue Can anybody help me cross-compile this project on Linux? #19 for details about this problem. Ultimately however, we'd want one build or make file to be the "single source of truth" in terms of building the project, whether through the IDE, through the command-line, or in a CI/CD pipeline like this one. Hopefully you or someone else can help me with that.
  • I need to update the Jemm version in the USB image, which is currently based on the official FreeDOS 1.3 Lite installer USB image. It seems that Jemm version v5.84pre2 would be the minimum required version for SBEMU to work. Is that correct?
    • Done. As of 2023-11-18, the pipeline in this PR now downloads Jemm v5.84pre2, adds it to the USB image and modifies fdconfig.sys so that Jemmex and the QPI Emulation JLM get loaded automatically when the USB image is booted.
  • I need to update the workflow to be triggered only on commits and merges to the main branch, but that is trivial to do. I have not yet configured that, since I've been testing the workflow in a separate branch, namely the one that I'm offering to be merged in this PR.
    • Update 2023-12-03: Done, as part of preparing the PR for final review.

But I think this workflow should provide a good starting point, and when the aforementioned questions have been answered, these improvement should be easy to make. What do you think?

Please let me know what changes (if any) you would like me to make before accepting this PR.

Thanks! 🙂

…play sources, to allow cross-platform compilation

Also correct `HTTP:\\` to `HTTP://` in a comment, which was probably the result of a general find/replace action by the Mpxplay developer(s)
…ile that contains a DJGPP based on much older GCC 6.1.0
…ng `GPR2MAK.EXE` utility in a DOSBox session
…written immediately using Etcher, without the user having to decompress it manually fist
@volkertb volkertb marked this pull request as ready for review December 3, 2023 15:45
@volkertb
Copy link
Collaborator Author

volkertb commented Dec 3, 2023

Alright @crazii, the MR is ready for final review (and hopefully approval 🤞). Thanks for your patience!

I've tested it thoroughly, and it's building releases on commit/push properly. The USB images are ready to use, and automatically load SBEMU on start-up.

This is what the automatic releases look like: https://github.com/volkertb/SBEMU/releases

In a future improvement, I'd like to bundle a script with the download, with which people can easily combine an image with DOS games with the SBEMU test image into a larger USB image that includes all the games. I don't want to do this automatically on each triggered build, because that would result in much bigger USB image sizes, and not everybody needs to have all those standard games included in the image.

@thp and @4nd3r50ncr feel free to take a look as well. If this gets merged into the SBEMU project, I'd like to add this (or something similar) to the sbemu-x fork as well.

Thanks! 🙂

…, to prevent DOS/4GW games from failing to launch with error `not enough memory to load program` when running on systems or VMs with more than 2GB RAM
Copy link
Collaborator

@thp thp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments. Thanks for working on this!

.github/workflows/01-build-and-release.yml Outdated Show resolved Hide resolved
.github/workflows/01-build-and-release.yml Outdated Show resolved Hide resolved
Comment on lines 34 to 37
wget https://www.ibiblio.org/pub/micro/pc-stuff/freedos/files/distributions/1.3/official/FD13-LiteUSB.zip
wget https://www.freedos.org/download/verify.txt
grep -q "64a934585087ccd91a18c55e20ee01f5f6762be712eeaa5f456be543778f9f7e FD13-LiteUSB.zip" verify.txt
echo "64a934585087ccd91a18c55e20ee01f5f6762be712eeaa5f456be543778f9f7e FD13-LiteUSB.zip" | shasum -a 256 --check
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of downloading verify.txt if the only thing we're doing with it is checking the hash contained within the file, only to then do the check without verify.txt:

echo "64a934585087ccd91a18c55e20ee01f5f6762be712eeaa5f456be543778f9f7e  FD13-LiteUSB.zip" | shasum -a 256 --check

I'd just remove the downloading and checking of verify.txt, as checking the hash of FD13-LiteUSB.zip is the important piece, and that's already done here (as it should be).

Also, if we do check the hash of the file (which is a good thing), why don't we check the hash of djgpp-linux64-gcc1210.tar.bz2 downloaded above?

.github/workflows/01-build-and-release.yml Outdated Show resolved Hide resolved
Comment on lines +92 to +123
<summary>Preparing a bootable USB drive</summary>

## Preparing a bootable USB drive

The USB image can be written to a USB drive or SD card using a tool like [balenaEtcher](https://etcher.balena.io/).

The advantage of using Etcher is that you don't have to decompress the `.xz` archive first.
It will decompress such files automatically, before writing the image to the target drive.
</details>
<details>
<summary>Booting the USB image in a virtual machine</summary>

## Booting the USB image in a virtual machine

You can run the image in a VM with QEMU as follows:

```shell
unxz SBEMU-FD13-USB.img.xz
qemu-system-i386 -drive file=SBEMU-FD13-USB.img,format=raw -device AC97
```

If you wish to test Intel HDA compatibility instead of ICHx AC'97 compatibility, replace `AC97` with `intel-hda` in the last command above.
On Linux, you can include the parameter `--enable-kvm` to run the VM with hardware-assisted virtualization.

If you prefer to use another hypervisor, such as VirtualBox or VMware, you may have to convert the raw image to a supported VM image format first:

```shell
unxz SBEMU-FD13-USB.img.xz
qemu-img convert -f raw -O vmdk SBEMU-FD13-USB.img SBEMU-FD13-USB.vmdk
```

**NOTE**: Although VMs can sometimes be useful during development, testing and debugging, you should not rely on those for actual hardware compatibility testing, since the sound cards that the hypervisors emulate are themselves merely approximations of actual hardware, and will not behave like the real thing in every single corner case.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like all of this would better be put into README.md or some other place, instead of putting all of this into the release text all the time. If nothing else, a .github/RELEASE-NOTES.md or something that gets sourced here might be better.

Also, once those get outdated, it will be hard to update, and people might not see them, so I'd rather we have a file in the repository that we can link to (and easily update), and the release notes should just have a link to that file for the most up-to-date information.

mpxplay/in_file.h Show resolved Hide resolved
id: tag
run: |
echo "release_tag=UserBuild_$(date +"%Y.%m.%d_%H-%M")" >> $GITHUB_OUTPUT
- name: Release FreeDOS SBEMU USB image
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the FreeDOS image is appreciated, why not build a small self-contained release that can be put onto an existing DOS install? Having to download the FreeDOS image and extracting the SBEMU bits sounds tedious if one already has a DOS setup of some kind.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need both.
The single release is essential, and the FreeDOS image will be extra good that helps people build a USB bootdisk easily.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, I'm not really in favor of putting abemu inside a pre-configured or pre-installed .img image, I think it's unnecessary, I think focusing on development would be much more important, if I'm useful, please add me as part. I tested, in FreeDOS it still occurs, some error related to hdpmi, a strange error causes the mouse to overload the memory. and the system crashes. In my NEDR-DOS, a variant of Open-DOS, this error does not occur, and it runs very well, I suggest you test it with the game Kings Quest V, you will see the error

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a coder I agree with you, we need focus on the dev & bugfixes. But in a user's view a pre installed img is good to have, and we have nothing to lose. Leave this to volkertb and we just focus on the rest.

@thp
Copy link
Collaborator

thp commented Dec 3, 2023

@thp and @4nd3r50ncr feel free to take a look as well.

Review done, please have a look.

If this gets merged into the SBEMU project, I'd like to add this (or something similar) to the sbemu-x fork as well.

As for sbemu-x, now that the big parts of it have been merged, there's not much left (from sbemu-x's README.md):

  • Nicer text-mode user interface
  • Additional sound card support
    • CMI 8338 / 8738 (untested)
  • Toggleable debug output on serial port (/DBG0, /DBG1, /DBG2)

If @crazii wants to, they can merge those parts into upstream SBEMU, and we can just drop sbemu-x (it has fulfilled its purpose). It's a friendly fork, and now that SBEMU is active again, maintaining the fork doesn't make too much sense IMHO.

…nto separate shell script, and run linter (shellcheck) on script before running it in workflow
…workflow, for further testing, due to various points of review feedback requiring further changes
@volkertb
Copy link
Collaborator Author

volkertb commented Dec 3, 2023

@thp I agree with your suggestion to merge the improvements in sbemu-x back into SBEMU. At the end of the day, we share the same goal(s) with both these projects. 🙂 Have you opened a GitHub issue yet with this proposal?

@crazii
Copy link
Owner

crazii commented Dec 3, 2023

@thp thanks for help reviewing the code, very much appreciated!

@crazii
Copy link
Owner

crazii commented Dec 5, 2023

There's two jobs with the same name in the workflow, the hello world one is not merged, right?
Have you tested the built image, is it working? I cannot find the artifacts to test.

@volkertb
Copy link
Collaborator Author

volkertb commented Dec 5, 2023

@crazii There is just one workflow file, but it used to be called 01-hello-world.yml. I renamed it to 01-build-and-release.yml about 2 weeks ago in commit 01e7701.

And yes, I've tested it a lot. The generated artifacts can be found and downloaded on the releases page of my (temporary) SBEMU fork, in which I created the branch that I am offering in this PR.

@crazii
Copy link
Owner

crazii commented Dec 6, 2023

OKay, I test it soon and merge the code.

@crazii
Copy link
Owner

crazii commented Dec 6, 2023

@thp and @4nd3r50ncr feel free to take a look as well.

Review done, please have a look.

If this gets merged into the SBEMU project, I'd like to add this (or something similar) to the sbemu-x fork as well.

As for sbemu-x, now that the big parts of it have been merged, there's not much left (from sbemu-x's README.md):

  • Nicer text-mode user interface

  • Additional sound card support

    • CMI 8338 / 8738 (untested)
  • Toggleable debug output on serial port (/DBG0, /DBG1, /DBG2)

If @crazii wants to, they can merge those parts into upstream SBEMU, and we can just drop sbemu-x (it has fulfilled its purpose). It's a friendly fork, and now that SBEMU is active again, maintaining the fork doesn't make too much sense IMHO.

Yes, it'll be good if those improvements merged to upstream SBEMU, please create a PR.

@crazii crazii merged commit 094b263 into crazii:main Dec 6, 2023
@Torinde
Copy link
Contributor

Torinde commented Dec 7, 2023

If @crazii wants to, they can merge those parts into upstream SBEMU, and we can just drop sbemu-x (it has fulfilled its purpose). It's a friendly fork, and now that SBEMU is active again, maintaining the fork doesn't make too much sense IMHO.

From SBEMU-X there is also a PR waiting for review at sbemu-x/pull/10, some additions to the README and a couple of Issues which don't seem to have equivalents here.

@volkertb
Copy link
Collaborator Author

volkertb commented Dec 7, 2023

Thank you for merging my PR, @crazii! I had been working on this for quite some time. I may not (yet) have your level of skill when it comes to actual low-level coding, but I'm glad I could contribute something. Hopefully it will be helpful to others. ☺️

@volkertb volkertb deleted the set-up-cicd branch December 7, 2023 23:20
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.

5 participants