I am concerned about the additional maintenance burden imposed. Whenever we change the package dependencies we will have to update and test the Docker image. I don't see that there are enough project resources for this.

Hi @uweseimet

Please use bullseye instead of buster. Buster is already outdated.

The rationale for using Buster is due to the backward compatibility requirement here.

I will look into adding support for Bullseye as well.

@nucleogenic Regarding buster and Bullseye, we have to be compatible with both.

The environment can now be targeted with the following env vars:

• OS_DISTRO [debian]
• OS_VERSION [buster]
• OS_ARCH [amd64]

These are used to derive the image names in docker-compose.yml and the base images (FROM) in the Dockerfiles.

For example:

Analogue of Raspberry Pi OS legacy:

docker compose up

Analogues of Raspberry Pi OS latest:

OS_VERSION=bullseye docker compose up
OS_VERSION=bullseye OS_ARCH=arm32v7 docker compose up

Ubuntu 22.04 LTS:

OS_DISTRO=ubuntu OS_VERSION=jammy docker compose up

For convenience, the default environment can be specified via docker-compose.override.yml if required.

Alternatives I considered to the env var approach were multiple docker-compose.[environment].yml files and/or multiple [environment].Dockerfile files, but since there is no deviation in the build sequence for each environment, I decided to minimise LOC for maintainability.

@nucleogenic Can you please comment on the maintenance burden? Who is going to maintain these images, and who is going to use them?

Hi @uweseimet,

I don't propose we use Docker as a distribution channel for releases; instead, this change set provides tooling to allow contributors to create their own local images, which can be used for development and testing purposes.

Certain parts of the project, such as the web UI and easyinstall.sh script, are quite tightly coupled with the target environment. They have specific requirements, e.g. Python 3.7 or 3.9, the ability to install packages via apt, sudo access, etc.

Currently, working on and testing these services means using the Raspberry Pi itself, or testing on a similar environment (computer or VM with Linux installed.) Both of these are time consuming as they are comparatively slow to provision. This is particularly true when wanting to test in a clean environment where the state has not already been modified by subsequent testing (example #803).

With this in mind, I posit there is already a significant maintenance burden in the form of manual testing to avoid regressions when any changes are made.

Introducing Docker will not solve this, of course, but I believe the ability to quickly get access to production-like environments is a step in the right direction. It reduces the effort required for manual testing, and could serve as the foundation for some automated functional testing of easyinstall.sh and the web UI. There is still a need for unit tests in both the C++ and Python code bases, but that is a separate issue.

Your questions:

Who is going to maintain these images?

I am happy to maintain the Docker implementation and address any issues raised on the issue tracker. I do not foresee particularly high demand here, as the audience is project contributors, rather than end users. Pedantically, this is not part of the project that would stop a release shipping or end users' enjoyment, if there were issues.

Who is going to use them?

• Contributors with the use cases described in the PR description
• Reviewers of PRs (particularly changes to web and terminal UIs)
• Future contributors that want a low barrier to entry

@nucleogenic IMHO the overhead of using Docker images for rascsi does not justify the effort and maintenance costs. Docker will not help with manual testing. There should not be manual testing at all. Unit tests and test automation are needed.
For testing the rascsi core (C++) you can test everything except the code that needs the rascsi hardware (this is a very small part of the code) with unit tests on any platform. The environment cannot really cause issues here, because the vast majority of the code is agnostic of the environment. The hardware-specific part needs the hardware and can only be tested with a Pi, and also not with a Docker image.
As far as the web UI is concerned @rdmark is the specialist for this. He will know better than me whether the Docker approach has benefits for testing the UI. My (hopefully reasonable) assumption is, that with the right unit test setup you do not need a web server or any special web environment at all, and can test the python code and the UI on any machine in an automated fashion. In the past I have worked on projects where the UI tests were running on headless servers. (There servers were often Docker images in a Kubernetes environment.)

Anyway, @akuker and @rdmark should have a closer look, I guess @akuker's automation approach is going to be based on GitHub actions.

I do appreciate your efforts, but as you may have noticed that I have doubts that your uses cases provide benefits for the rascsi project. ;-) Maybe I am just missing the obvious ...

@akuker I suggest to ad a note on the website, saying that for contributions witth a potential major impact, or for changes that require more than just a minor effort, there has to be a ticket first. This provides the opportunity to learn about and discuss these things first. PRs that appear from nowhere and deal with major changes are not that pleasant IMHO ;-).

Hi @uweseimet,

IMHO the overhead of using Docker images for rascsi does not justify the effort and maintenance costs.

What do you consider the effort and maintenance costs to be in practice? This seems to be the basis of contention with this PR, and I would like to find a solution.

Docker will not help with manual testing.

Having access to fast environments covering Buster and Bullseye, on Python 3.7 and Python 3.9, is advantageous. This is based on my own experience working on the project, where time is consumed pushing code to an RPi, or working around OS-specific requirements (i.e. web/start.sh fails if certain binaries are missing, such as genisoimage), or setting up pyenv to be able to develop against 'outdated' Python 3.7 and Python 3.9 on a modern macOS.

There should not be manual testing at all.

Automated tests only cover the cases tests that exist. With the absence of high levels of test coverage (C++: unknown, Python: 0%, easyinstall.sh: unknown), I believe manual testing remains relevant on this project.

Even with high test coverage, automated testing does not cover exploratory testing, or the quality and consistency of user experience. A rather contrived example: a blue button on a blue background; an automated would find the button based on the text and click it - the test passes; a person would fail this test. We build software for people and should test it as such. This is especially true for user-facing areas of the project; I agree other areas can rely more heavily on unit tests.

Unit tests and test automation are needed.

I also agree, but at the time of writing, there are not enough tests to give high levels of assurance. Sometimes high-level tests (browser automation, or manual) are the quickest way to get assurance. We cannot refactor the entire codebase to be testable, and write all of the tests, for the next release. What we can do is reduce the burden of manual testing right now, and perhaps get some high-level automated tests in place.

--

Thanks for taking the time tor review my PR. Although I don't agree with your position on this issue, I do appreciate your time and the stewardship shown towards RaSCSI. I hope we can find a solution that works for both of us.

Hi @uweseimet - This change did not come out of nowhere. This change has been discussed HEAVILY in the development discord channel. You are very welcome to join in that conversation.

I don't understand the maintenance concerns with this change? This feature is for web UI development only. It will not be included in the pre-built image and should not be automatically installed with easyinstall.sh. If we decided in the future that this is no longer needed or becomes out of date, its extremely easy to remove docker support. rm -rf ./docker

The web interface has become an extremely important feature to the RaSCSI users. I have absolutely no hesitation about pulling in docker support for development purposes if it helps improve that feature. This change might have little to no benefit to the C++ development process. However, that is only half of this project. If you don't see value in having a docker container for as part of your workflow, my recommendation is to just not use it. Other people do see value.

@nucleogenic - I sincerely appreciate this pull request and am glad to have you as part of this community. I hope we can do better in being a more welcoming environment to other people's ideas.

@akuker Well, I pointed out that others can better comment on the UI. I apologize if my comments were offending.

Regarding Discord, anything that is not temporary but has a longer lasting impact should have a ticket. Discord and similar tools are more a fire and forget thing, and are not intended to hold information that may still be relevan in, let's say, a few months.

Hi @uweseimet

I can't merge this branch until your 'changes requested' flag is satisfied.

I have added env vars to allow the environment to be built with Debian buster and bullseye as the base image.

Are there any other changes you would like me to make?

Thanks!

@nucleogenic No, no other changes. Sorry, I did not know that I have to unblock. I will immediately check this.

Thanks @uweseimet

@akuker Are we OK to merge this? (I'm not aware of the policy on merges.)

I have squashed the branch following completion of the reviews.

Patch to cfilesystem.cpp is outdated following rebase, returning to draft until I've updated this.

@nucleogenic Regarding the commented out code: Please remove code that is not needed anymore. Otherwise please add a TODO saying why this code is commented out.

Hi @uweseimet

I've added an extra comment to the patch file: https://github.com/akuker/RASCSI/pull/819/files#diff-e4676285219b05b88de3806b592a65052ef9e8e7da799e22cc59db35cbf1f0ceR13-R14

There's already one in the rascsi/Dockerfile which applies the patch.

@nucleogenic I don't think we can comment out code and change the logic. The modified code will be broken after commenting some lines out, won't it? If there were unit tests for this code they would also fail.
If there is a compile-time issue on a non-supported platform/compiler (in particular a platform where rascsi hardware is not available for), and this issue cannot be resolved, we cannot support this platform. Not absolutely sure whether the compiler is right or not. (It appears to be the only one reporting this).

@akuker Maybe I am missing something here, but IMO this change breaks the code, doesn't it? Looks as if this change was merged into develop 2 hours ago without any review?

Hi @uweseimet,

There is no currently known use case for the code that is modified by the patch (X68000 related) in the Docker environment. Please let me know if you have one.

Applying a patch at build time to a copy of the source code, inside the container, felt like a sensible workaround to allow RaSCSI to compile on Debian "bullseye" (amd64), without making changes to the C++ code in the repository.

The patch solution was present when both @akuker and yourself approved the changes, and was only updated to take into account the changes from rebasing, plus adding the comment as you requested.

I did attempt to solicit help in #821 for a cleaner fix of the root issue, but the issue was rejected after it had only been open for a few hours, so it didn't receive much visibility.

I would like to summarize from my perspective the two things that happened today and should not happen. As already mentioned, I may be missing something, so please bear with me. From looking at the PRs this is what I find:

1. The CD-ROM change was merged even though changes had been requested, and these changes had not been reviewed by the developer who requested them. The code was essentially fine, but there was one unused parameter, which now causes the -O3 build to fail.
2. There is a compile time issue on a non-Pi platform. The offending code was commented out and the existing logic was changed. Some of these changes were committed without having been approved by anybody.

@nucleogenic You mean the patch is only applied in the container? I was not aware of that. In this case I don't have any issue with it, of course. But a change was committed without having been approved, wasn't it?
Just trying to find out what happened.

By the way, the code in cfilesystem.cpp is a nightmare. (SonarCloud also thinks so.) I hardly dare to touch it. Would like to get rid of it ;-).

@uweseimet

I am unsure what you are referring to re: the CD-ROM change. My change set was rebased off develop before merge. If I made a mistake here, please let me know so I can understand what happened?

The last change to the patch file (but not the implementation approach) was merged without an explicit approval on that commit. GitHub should be updated to reset the review status on updates, if this is the policy. Unfortunately, in the absence of documented policies and procedures for contributors, once must rely on their reasonable judgement.

There is an issue to evaluate whether the X68000 code is retained at #830.

@nucleogenic The CD-ROM change is related to #835. This is what currently breaks building the develop branch with optiimzation -O3.

For thoroughness, the final changes to cfilesystem.patch prior to merge can be found in the attached diff here.