mviereck / x11docker

Run GUI applications and desktops in docker and podman containers. Focus on security.
MIT License
5.59k stars 375 forks source link

refactor: split the main script into multiple files, use template files #388

Closed milahu closed 2 years ago

milahu commented 2 years ago

Due to the size of x11docker (9k lines of code) it is a bit difficult to debug and maintain it for others than myself.

is probably the main issue in #353

see my solution to use bash to generate bash script from template these template files (*.tpl.sh) are MUCH easier to read and maintain than template-strings

i suggest to move all the branching code (if/else/case) into the template files, and use only the template variables for branching

currently, im extracting the code for Xephyr, to create a minimalistic xephyr-docker

mviereck commented 2 years ago

I don't see an advantage in splitting x11docker into multiple files. The core code flow is visible in main(), and that is not much to read.

For reading and debugging it helps a lot to use an editor with code folding. The editor of my choice is kate that provides code folding and good syntax highlighting.

An advantage of a single file is simple redistribution. One can just add this single script to his Dockerfile or image.

One split I thought of is to divide the X server part from the container part. So there would be a separate script to run X servers. Currently runx does this on MS Windows.

milahu commented 2 years ago

An advantage of a single file is simple redistribution.

we could bundle the split files into one file, like a javascript bundler (webpack, rollup, vite, ...)

For reading and debugging it helps a lot to use an editor with code folding.

i want syntax highlighting of the generated bash scripts, and i want to remove the escape-hell of template-strings

eine commented 2 years ago

I believe it is not worth splitting x11docker into multiple files while still using bash. It would make sense to have the code structured differently if Python was used. That would make the effort worth because it would provide additional capabilites which Python supports but are difficult to achieve in bash (nested lists/dictionaries, classes, inheritance, JSON/YAML manipulation...). In fact, since recently, x11docker relies on Python for parsing some docker output (see https://github.com/mviereck/x11docker/blob/master/x11docker#L930). Hence, it is the natural evolution.

mviereck commented 2 years ago

In fact, since recently, x11docker relies on Python for parsing some docker output

Basically this was needed to support --backend=nerdctl #357. However, it made some parsing for docker and podman cleaner and easier, too.

It would make sense to have the code structured differently if Python was used. That would make the effort worth because it would provide additional capabilites which Python supports but are difficult to achieve in bash (nested lists/dictionaries, classes, inheritance, JSON/YAML manipulation...).

A rewrite in python3 would make most sense (if at all). I just don't see a need because x11docker works as it is.

eine commented 2 years ago

I just don't see a need because x11docker works as it is.

Agree. I do believe that a Python3 module with the X server stuff separated from the container specific layer would be (re)usable by several projects, so it might potentially slightly improve the popularity of x11docker. However, as commented in some other issue, that's a several week effort (if not months). It does only make sense for someone who wants to learn about either bash or Python, as an exercise with a useful outcome.

mviereck commented 2 years ago

Agree. I do believe that a Python3 module with the X server stuff separated from the container specific layer would be (re)usable by several projects, so it might potentially slightly improve the popularity of x11docker.

Basically it is already possible to use x11docker only to provide an X server. But I assume close to no one is aware of that. A separate project only for this task might get some popularity.

Just splitting the X server part out of x11docker in bash wouldn't be too hard (at least for me). But than it is no longer a single script.

Example:

x11docker --desktop --exe -- startlxde

would start LXDE in a new X server. Several options to choose and adjust the X server are available.

eine commented 2 years ago

Basically it is already possible to use x11docker only to provide an X server. But I assume close to no one is aware of that. A separate project only for this task might get some popularity.

@mviereck, see section "Gitpod" in https://github.com/hdl/containers/issues/45. That's what I had in mind. Note that Gitpod is integrated into GitLab (https://docs.gitlab.com/ee/integration/gitpod.html). At some point, I would expect Microsoft to do something similar in GitHub.

milahu commented 2 years ago

x11docker relies on Python for parsing some docker output

also xpra requires python

but nevermind, im moving on to firecracker and ignite

let me close this as "too much pain, too little gain"

mviereck commented 2 years ago

see section "Gitpod" in hdl/containers#45. That's what I had in mind.

I am not exactly sure what you want to point me on. If I understand right that section tells of GUIs in containers and cites x11docker. Would there be a need for a pure X server tool?

eine commented 2 years ago

Gitpod is a service that runs an container in a server and starts a VSCode or Theia instance. It allows clicking a button in a repository and having a ready-to-use IDE in a custom environment (the container and the cloned repo). When users need to use GUI tools in that setup, the default approach is installing VNC in the container. I believe it should be possible to use x11docker with authentication features enabled for using an X server or xpra instead of VNC.

mviereck commented 2 years ago

I believe it should be possible to use x11docker with authentication features enabled for using an X server or xpra instead of VNC.

I thought that it was this what @umarcor wanted to say there.

milahu commented 2 years ago

let me close this as "too much pain, too little gain"

for what its worth ... here is my attempt at refactoring if anyone wants to port x11docker to python (or whatever), maybe this is a useful start

i removed the function-redirects for better readability (and cos tree-sitter-bash parsed them wrong)

new x11docker script

all functions moved to lib/ (added some of my temp files by accident, e.g. *.txt and *.2.sh)

sample use of fsed1file (template processor) in lib/alertbox.sh

sample result of extract_script.sh = call a x11docker-subfunction, write output to file, generate call to fsed1file lib/create_dockerrc.tpl.sh lib/create_dockerrc.call-tpl.sh

the manual work: set switching vars like

Debugmode="yes"
Containersetup="yes"
Containerenvironmentcount=1 # lib/store_runoption.sh
Sharevolumescount=1
Containerenvironment=("TODO__Containerenvironment_idx_0__TODO")
Sharevolumes=("TODO__Sharevolumes_idx_0__TODO")

... and move ALL switching code into the *.tpl.sh file

alternative approach: parse the x11docker-subfunction, process the echo calls problem is: i have not-yet found a bugfree parser. closest is https://github.com/tree-sitter/tree-sitter-bash sample bug in tree-sitter-bash: https://github.com/tree-sitter/tree-sitter-bash/issues/108 bashlex for python has lots of "not implemented" code

i removed the function-redirects for better readability (and cos tree-sitter-bash parsed them wrong)

this commit got lost ... i mean:

f() {
  echo hello
} >output.txt
f

to

f() {
  echo hello
}
f >output.txt
mviereck commented 2 years ago

Thank you for explaining your thoughts and attempts!

i removed the function-redirects for better readability (and cos tree-sitter-bash parsed them wrong)

That's a good suggestion, I've changed x11docker accordingly.

new x11docker script all functions moved to lib/

The new x11docker script looks pretty much same here in kate with code folding. I'd say splitting code into multiple files is not a value by itself. It makes sense if the separated files can be used independently on their own. This is the case only for a few helper functions yet.

It would be possible to write more independent/modular functions. Currently many of them rely heavily on global variables, what is not really great. In a major rewrite it would be a possible way to write more modular functions that print some actually needed code. For example, a sound() function could print (by switch) docker command parts or pulseaudio config files. In a similar way a gpu() function could print code for docker command and code for nvidia driver installation in container.

sample use of fsed1file (template processor) in lib/alertbox.sh sample result of extract_script.sh = call a x11docker-subfunction, write output to file, generate call to fsed1file lib/create_dockerrc.tpl.sh lib/create_dockerrc.call-tpl.sh

This part I still try to understand. It is nice to have one escape level less than before, but in change another complexity is added.

milahu commented 2 years ago

It is nice to have one escape level less than before, but in change another complexity is added.

yepp ... its complex in bash, where "everything is a string", and passing structured data is non-trivial (json, jq) i figured, when porting x11docker to python, this "extract scripts" step is needed anyway, so i shared my start