-
Notifications
You must be signed in to change notification settings - Fork 55
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
Move default-settings to /etc/xpra #111
Conversation
@totaam :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had forgotten about this PR.
setup.py
Outdated
@@ -513,6 +524,17 @@ def main(args): | |||
if len(args)>=4: | |||
minifier = args[3] | |||
install_html5(install_dir, minifier) | |||
if len(args)<3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only if there are less than 3 args?
Shouldn't this be done in all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
setup.py
Outdated
# 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will fix it!
setup.py
Outdated
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of repeating the same path 4 times, please use a variable to make the code more readable. ie: www_dir
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be fixed too
setup.py
Outdated
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
setup.py
Outdated
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" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
Simply not having the .gz
and .br
is cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix
b406279
to
909d171
Compare
@totaam The new revision ships only |
packaging/debian/changelog
Outdated
@@ -1,4 +1,4 @@ | |||
xpra-html5 (4.5.2-r1075-1) UNRELEASED; urgency=low | |||
xpra-html5 (4.5.2-r214-1) UNRELEASED; urgency=low |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these changes from the PR.
(it's unrelated and for some reason, it's going backwards?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, it is an artifact from testing the build! Sorry! :)
setup.py
Outdated
# 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" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's settle with Configuration files that must not be minified and compressed…
and not_minify_files
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK!
I would use an array: configuration_files = ["default-settings.txt"]
and not_minify_files
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! Then let me push onve more ;)
setup.py
Outdated
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated, I've rolled it into: 0c3ba3b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
d9b52b9
to
dc4e705
Compare
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>
dc4e705
to
978d9f4
Compare
Just noticed:
That won't work for
And the RPMs don't have I will also have to ensure this doesn't cause problems for the MS Windows and MacOS builds... |
There are different paths. make_deb operates from
Just use |
MS Windows is not POSIX, macos is posix but it has |
Our builds are based on MSYS2 (mingw64) so they are actually a lot more posixy than you would think.
Not one we can use. |
How? |
If |
That would not work for |
Then let me refactor this into a sysroot helper? i.e if install directory is specified, treat is as plain directory ( Can you please guide me about your preferences on related systems? i,e which system has default-settings where? |
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