search

GeorgeStefanChira / epics_docker_setup

MIT License
0 stars 0 forks source link

Review comments #2

Open ulrikpedersen opened 3 months ago

ulrikpedersen commented 3 months ago

We don't have a pull request open for the review of this one so I'll just use this issue ticket to track comments from reviewing what is currently on the main branch.

ulrikpedersen commented 3 months ago

Dockerfile

You get the EPICS base dependencies installed 👍 However, I think you're missing setting of a few environment variables that I suspect must have caused some issues? Or maybe because you're on Linux and x86_64 is just defaults to sort of work - but might not work for other platforms.

Best practice is to set things like EPICS_HOST_ARCH and EPICS_BASE variables in the environment as described in the installation instructions: https://docs.epics-controls.org/en/latest/getting-started/installation-linux.html

I would recommend downloading the EPICS release tarball instead of cloning the git repo.

On L52 onwards: you can probably copy the whole structure of the tree instead of individual files?

General good practice in Dockerfiles is to avoid running them as root whenever possible. By default you're root when building the docker image - and you need that in order to install software dependencies. But then you can create a non-root user and switch to that when you no longer need root (you might have to change permissions on your EPICS install dir). Then when a user runs the container they are not root.

ulrikpedersen commented 3 months ago

module_installer.py

A lot of good practice python in here 👍

A few things I would recommend to take a slightly different approach to:

Using os.path is better than just using strings to represent files and paths. However, the Python pathlib and *Path classes are a more modern approach to dealing with paths.

If you use pathlib.Path then you also don't need to add f"{current_directory} ... to everything as it can give you the absolute path from a relative path if you need it. It would make the code a little less verbose, especially in check_path_to_modules()

ulrikpedersen commented 3 months ago

README.md

Good intro to the module: clear instructions on how to build and run the container with the IOC.

Perhaps missing a little bit of information about how to use this in a runtime environment on a host. For example: what type of network configuration does your docker container need to run with on the host in order to allow EPICS Channel Access to work in/out of the container? I.e. if the IOC is in the container and the client (phoebus or just CLI caget) is on the host or on another computer on the LAN?

Have you run into the issue about not being able connect via CA from host to container-IOC @GeorgeStefanChira ?

ulrikpedersen commented 3 months ago

Overall a good implementation of the IOC in a container 👍

Next steps would be to adopt the (much more complex) epics-containers framework that essentially do the same thing - but takes it much further. This project also provides pre-built images with EPICS base and a lot of common support modules, but the learning curve is rather steep... (we won't start that right now as we have other priorities right now...)