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

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

Closed
milahu opened this issue Oct 7, 2021 · 14 comments
Closed

Comments

@milahu
Copy link

milahu commented Oct 7, 2021

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
Copy link
Owner

mviereck commented Oct 7, 2021

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
Copy link
Author

milahu commented Oct 7, 2021

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
Copy link
Contributor

eine commented Oct 8, 2021

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
Copy link
Owner

mviereck commented Oct 8, 2021

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
Copy link
Contributor

eine commented Oct 8, 2021

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
Copy link
Owner

mviereck commented Oct 8, 2021

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
Copy link
Contributor

eine commented Oct 8, 2021

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 hdl/containers#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
Copy link
Author

milahu commented Oct 8, 2021

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"

@milahu milahu closed this as completed Oct 8, 2021
@mviereck
Copy link
Owner

mviereck commented Oct 8, 2021

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
Copy link
Contributor

eine commented Oct 8, 2021

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
Copy link
Owner

mviereck commented Oct 8, 2021

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
Copy link
Author

milahu commented Oct 9, 2021

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: tree-sitter/tree-sitter-bash#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
Copy link
Owner

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
Copy link
Author

milahu commented Oct 10, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants