Move default-settings to /etc/xpra #111

basilgello · 2021-10-28T09:29:59Z

The default-settings.txt file is a configuration file
editable by user, so its place on Unix systems is
in /etc.

Also fix typo in make_rpm

basilgello · 2021-10-28T09:31:21Z

@totaam :)

totaam

Sorry, I had forgotten about this PR.

totaam · 2021-11-03T12:40:00Z

setup.py

@@ -513,6 +524,17 @@ def main(args):
        if len(args)>=4:
            minifier = args[3]
        install_html5(install_dir, minifier)
+        if len(args)<3:


Why only if there are less than 3 args?
Shouldn't this be done in all cases?

I recall I did it to preserve default-settings.txt inside install_dir if the installation directory is specified by user. This should keep the logic for bdist intact.

totaam · 2021-11-03T12:40:54Z

setup.py

+    # Move and symlink default-settings.txt
+    os.makedirs("./xpra-html5/etc/xpra/html5-client", exist_ok=True)
+    default_settings_files = { "default-settings.txt", "default-settings.txt.gz", "default-settings.txt.br" }
+    for filename in default_settings_files:


I don't like having 3 copies of what is effectively the same file in /etc
We should just prevent this file from ever being minified instead.

Yes, I will fix it!

totaam · 2021-11-03T12:45:23Z

setup.py

+    for filename in default_settings_files:
+        print("Checking existence of %s" % os.path.join("./xpra-html5/usr/share/xpra/www", filename))
+        if os.path.exists(os.path.join("./xpra-html5/usr/share/xpra/www", filename)):
+           os.rename(os.path.join("./xpra-html5/usr/share/xpra/www", filename),


Instead of repeating the same path 4 times, please use a variable to make the code more readable. ie: www_dir.

This will be fixed too

totaam · 2021-11-03T12:51:16Z

setup.py

+           os.rename(os.path.join("./xpra-html5/usr/share/xpra/www", filename),
+               os.path.join("./xpra-html5/etc/xpra/html5-client", filename))
+           os.symlink(os.path.join("/etc/xpra/html5-client", filename),
+               os.path.join("./xpra-html5/usr/share/xpra/www", filename))


Why not just symlink this file from www_dir directly to /etc/xpra/html5-client/?
(and since it should be just one file - is the html5-client directory even needed?)

The idea of whole change os that configuration files must reside in sub-directories of /etc, so symlink in /usr/share/xpra/www points to editable /etc/xpra/default-settings.txt. Separate directory for html5-client is not needed if we rename the file like html5-client-settings.txt.

totaam · 2021-11-03T12:52:37Z

setup.py

+        if len(args)<3:
+            # Move and symlink default-settings.txt
+            os.makedirs(os.path.normpath(os.path.join(sys.prefix, "../etc/xpra/html5-client")), exist_ok=True)
+            default_settings_files = { "default-settings.txt", "default-settings.txt.gz", "default-settings.txt.br" }


As above.
Simply not having the .gz and .br is cleaner.

basilgello · 2021-11-05T07:13:06Z

@totaam The new revision ships only default-settings.txt. I decided (for now) to ship it in /etc/xpra/html5-client/ because we need either to rename file in /etc/xpra/ to something like html5-client-settings.txt or to add migration code for existing default-settings.txt files installed without package managers like rpm or dpkg.

totaam · 2021-11-05T10:28:55Z

packaging/debian/changelog

@@ -1,4 +1,4 @@
-xpra-html5 (4.5.2-r1075-1) UNRELEASED; urgency=low
+xpra-html5 (4.5.2-r214-1) UNRELEASED; urgency=low


Please remove these changes from the PR.
(it's unrelated and for some reason, it's going backwards?)

Oops, it is an artifact from testing the build! Sorry! :)

totaam · 2021-11-05T10:30:34Z

setup.py

+# These must not be minified or compressed and must be moved to /etc/xpra
+# and symlinked to /usr/share/xpra/www if no custom installation directory
+# is set
+configuration_files = { "default-settings.txt" }


You say this is a List of configuration files but it is actually a set...
Then later on, you call it a not_minify_dict, but it isn't a dict.

Let's settle with Configuration files that must not be minified and compressed… and not_minify_files then?

OK!
I would use an array: configuration_files = ["default-settings.txt"] and not_minify_files.

OK! Then let me push onve more ;)

totaam · 2021-11-05T10:41:26Z

setup.py

@@ -456,7 +475,7 @@ def make_rpm():
    SOURCES = RPMBUILD+"/SOURCES"
    if not os.path.exists(SOURCES):
        os.makedirs(SOURCES, 0o755)
-    os.rename("dist/" + tarxz, SOURCES + tarxz)
+    os.rename("dist/" + tarxz, os.path.join(SOURCES, tarxz))


This change is unrelated, I've rolled it into: 0c3ba3b

The default-settings.txt file is a configuration file editable by user, so its place on Unix systems is in /etc. With this commit, configuration file with names defined in 'configuration_files' dict are not minified or compressed. They are moved to '/etc/xpra' directory if DEB or RPM package is built. Test: 1. Spawn container with Debian: podman run --rm -it -v $PWD:/work:rw \ docker.io/amd64/debian:sid /bin/bash 2. Inside container: export DEBIAN_FRONTEND=noninteractive && \ apt-get update && \ apt-get full-upgrade -yq && \ apt-get install -yq git python3-setuptools && \ cp -a /work /build && find /build -type d -exec chmod 0755 '{}' \; && \ cd /build && \ git checkout '*' && \ git clean -fdx && \ rm -rf /tmp/xpra-html5 && \ rm -rf /etc/xpra /usr/share/xpra/www && \ rm -rf /root/rpmbuild && \ python3 setup.py sdist && \ python3 setup.py install /tmp/xpra-html5 && \ python3 setup.py install && \ python3 setup.py deb && \ echo "Contents of dist tars:" && \ tar tvf dist/*.tar.gz && \ echo "Contents of /tmp/xpra-html5:" && \ ls -la /tmp/xpra-html5 && \ echo "Contents of /etc/xpra/html5-client /usr/share/xpra/www:" && \ ls -la /etc/xpra/html5-client /usr/share/xpra/www && \ echo "Contents of Debian packages:" && \ find dist/ -name "*.deb" -exec dpkg -c '{}' \; && \ echo "OK" || echo "FAIL" Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

totaam · 2021-11-05T11:19:42Z

Just noticed:

conf_dir = os.path.normpath(os.path.join(sys.prefix, "../etc/xpra/html5-client"))

That won't work for sys.prefix = "/usr/local" as found on FreeBSD:

> os.path.join("/usr/local", "../etc/xpra/html5-client")
'/usr/local/../etc/xpra/html5-client'

And the RPMs don't have /etc/xpra/html5-client. Can you remind me why the logic is both in if cmd=="install": and make_deb? Why not just roll it into install_html5 just once?

I will also have to ensure this doesn't cause problems for the MS Windows and MacOS builds...

basilgello · 2021-11-05T11:35:33Z

Can you remind me why the logic…

There are different paths. make_deb operates from ./xpra-html5 as sysroot while install operates on /

as found on FreeBSD:

Just use ../../ instead?

basilgello · 2021-11-05T11:36:06Z

the MS Windows and MacOS builds

MS Windows is not POSIX, macos is posix but it has /etc.

totaam · 2021-11-05T12:43:57Z

MS Windows is not POSIX

Our builds are based on MSYS2 (mingw64) so they are actually a lot more posixy than you would think.
And we do ship an /etc directory in the application image. But since there is no proper symlink support and /etc is not in a user editable location. (although there are other user-editable locations, like the ones we use for the ssl certs)
Anyway let's not bother.

macos is posix but it has /etc.

Not one we can use.
Again, there are other locations, but let's not bother.

totaam · 2021-11-05T12:44:23Z

Just use ../../ instead?

How?

basilgello · 2021-11-05T14:07:29Z

conf_dir = os.path.normpath(os.path.join(sys.prefix, "../../etc/xpra/html5-client")) and wrap it to not win32 ?

If sys.prefix is /usr it will still evaluate to /etc/xpra, because second .. will be chewed away

totaam · 2021-11-05T14:09:59Z

That would not work for sys.prefix = /usr/local/ which should be using /usr/local/etc.

basilgello · 2021-11-05T14:13:17Z

Then let me refactor this into a sysroot helper? i.e if install directory is specified, treat is as plain directory (sdist), otherwise for windows, for freebsd, else…

Can you please guide me about your preferences on related systems? i,e which system has default-settings where?

totaam requested changes Nov 3, 2021

View reviewed changes

basilgello force-pushed the default-settings-conffile branch from b406279 to 909d171 Compare November 5, 2021 06:00

basilgello requested a review from totaam November 5, 2021 06:54

totaam reviewed Nov 5, 2021

View reviewed changes

basilgello force-pushed the default-settings-conffile branch 2 times, most recently from d9b52b9 to dc4e705 Compare November 5, 2021 10:47

basilgello force-pushed the default-settings-conffile branch from dc4e705 to 978d9f4 Compare November 5, 2021 10:51

totaam merged commit eb3413e into Xpra-org:master Nov 5, 2021

basilgello mentioned this pull request Nov 5, 2021

Stuttering and off-sync rendering when other encodings than jpeg are used #92

Closed

basilgello mentioned this pull request Nov 5, 2021

Refactor installation and configuration directory selection #112

Merged

totaam added a commit that referenced this pull request Dec 5, 2021

#111 symlink should not point inside buildroot

6ce8c74

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move default-settings to /etc/xpra #111

Move default-settings to /etc/xpra #111

basilgello commented Oct 28, 2021

basilgello commented Oct 28, 2021

totaam left a comment

totaam Nov 3, 2021

basilgello Nov 4, 2021

totaam Nov 3, 2021

basilgello Nov 4, 2021

totaam Nov 3, 2021

basilgello Nov 4, 2021

totaam Nov 3, 2021

basilgello Nov 4, 2021

totaam Nov 3, 2021

basilgello Nov 4, 2021

basilgello commented Nov 5, 2021

totaam Nov 5, 2021

basilgello Nov 5, 2021

totaam Nov 5, 2021

basilgello Nov 5, 2021

totaam Nov 5, 2021

basilgello Nov 5, 2021

totaam Nov 5, 2021

basilgello Nov 5, 2021

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021

basilgello commented Nov 5, 2021

totaam commented Nov 5, 2021

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021 •

edited

Loading

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021 •

edited

Loading

		@@ -1,4 +1,4 @@
		xpra-html5 (4.5.2-r1075-1) UNRELEASED; urgency=low
		xpra-html5 (4.5.2-r214-1) UNRELEASED; urgency=low

Move default-settings to /etc/xpra #111

Move default-settings to /etc/xpra #111

Conversation

basilgello commented Oct 28, 2021

basilgello commented Oct 28, 2021

totaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basilgello commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021

basilgello commented Nov 5, 2021

totaam commented Nov 5, 2021

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021 • edited Loading

totaam commented Nov 5, 2021

basilgello commented Nov 5, 2021 • edited Loading

basilgello commented Nov 5, 2021 •

edited

Loading

basilgello commented Nov 5, 2021 •

edited

Loading