fix:Regenerate guake schema if old schema file found #1893

Davidy22 · 2021-09-13T02:17:06Z

Currently guake only generates a schema file if there is no schema file present. If there is a schema file from an older version of guake present, glib will attempt to use that file, segfault and never return execution back to python. Do manual version recording and checking ourselves to regenerate the file instead of relying on exception handling on glibc.

Fixes #1718, and I think should resolve #1621 as well.

mlouielu

Some points I want to know more about it.

guake/guake_app.py

mlouielu · 2021-09-13T02:47:06Z

guake/paths.py.in

@@ -94,3 +94,5 @@ AUTOSTART_FOLDER = {{ AUTOSTART_FOLDER }}
 def try_to_compile_glib_schemas():
    log.info("Compiling schema: %s", SCHEMA_DIR)
    subprocess.check_call(["glib-compile-schemas", "--strict", SCHEMA_DIR])
+    with open(os.path.join(SCHEMA_DIR, "version"), "w", encoding="utf-8") as version:
+        version.write(guake.guake_version())


Can this detect dirty version?

e.g. when you are doing some scheme change, will this embed something like 3.7.1-ea39dkf-dirty before you bump Guake version?

guake_version() in dev appends ".dev[numbers]" to the end of the version number so there is distinction, but this is mostly moot for work in dev because sudo make install correctly creates the schema file always before we even get to this line, this patch only really affects people trying to install from pip or wheels, which only have clean version numbers.

thanks for clarify

mlouielu · 2021-09-13T02:47:51Z

guake/guake_app.py

@@ -101,14 +101,25 @@ class Guake(SimpleGladeApp):
    def __init__(self):
        def load_schema():
            log.info("Loading Gnome schema from: %s", SCHEMA_DIR)
+            with open(os.path.join(SCHEMA_DIR, "version"), encoding="utf-8") as version:


Prefer to use pathlib (as we only support >= 3.6)

with open(pathlib.Path(SCHEMA_DIR / "version"), ...)

Wasn't os.path.join already around before 3.6? I'll change it to pathlib anyways

yep, but pathlib will handle any annoying file system stuff (although Guake didn't run on different OS...)

Changed to pathlib

mlouielu · 2021-09-13T02:50:06Z

guake/paths.py.in

@@ -94,3 +94,5 @@ AUTOSTART_FOLDER = {{ AUTOSTART_FOLDER }}
 def try_to_compile_glib_schemas():
    log.info("Compiling schema: %s", SCHEMA_DIR)
    subprocess.check_call(["glib-compile-schemas", "--strict", SCHEMA_DIR])
+    with open(os.path.join(SCHEMA_DIR, "version"), "w", encoding="utf-8") as version:


Prefer pathlib, and I don't think we need encoding="utf-8", they won't contain unicode codepoint character.

Including encoding in open() calls makes the linter happy. I'll change it to ascii.

emit by flake8?

I changed the explicit encoding to ascii already. Probably don't want to remove the check from our linter, it's an alright check

Makefile

gsemet · 2021-09-13T05:28:48Z

guake/guake_app.py

@@ -101,14 +101,25 @@ class Guake(SimpleGladeApp):
    def __init__(self):
        def load_schema():
            log.info("Loading Gnome schema from: %s", SCHEMA_DIR)
+            with open(Path(SCHEMA_DIR, "version"), encoding="ascii") as version:


Not sure if this is a good idea, when installed on the system I remember well all gschema can be placed into a single directory.

maybe the name of the schéma file + version. Or burn the guake version into the schéma file in a new key

Didn't consider using a param in the schema file. Changed approach to use that. No additional version tracking files now.

gsemet · 2021-09-13T05:29:46Z

guake/paths.py.in

@@ -94,3 +96,5 @@ AUTOSTART_FOLDER = {{ AUTOSTART_FOLDER }}
 def try_to_compile_glib_schemas():
    log.info("Compiling schema: %s", SCHEMA_DIR)
    subprocess.check_call(["glib-compile-schemas", "--strict", SCHEMA_DIR])
+    with open(Path(SCHEMA_DIR, "version"), "w", encoding="ascii") as version:
+        version.write(guake.guake_version())


I think you will have to use the same file name + extension to avoid any conflict

Currently guake only generates a schema file if there is no schema file present. If there is a schema file from an older version of guake present, glib will attempt to use that file, segfault and never return execution back to python. Do manual version recording and checking ourselves to regenerate the file instead of relying on exception handling on glibc.

Davidy22 · 2021-09-13T12:43:39Z

Changed the approach somewhat to use a setting in the schema file.

eli-schwartz · 2021-09-13T12:57:01Z

Makefile

@@ -142,7 +141,7 @@ install-schemas:
 	install -Dm644 "$(DEV_DATA_DIR)/org.guake.gschema.xml" "$(DESTDIR)$(SCHEMA_DIR)/"

 compile-shemas:
-	if [ $(COMPILE_SCHEMA) = 1 ]; then glib-compile-schemas $(DESTDIR)$(gsettingsschemadir); fi
+	glib-compile-schemas $(DESTDIR)$(gsettingsschemadir)


Distro packages MUST NOT have a compiled schema as this is handled globally outside of any package. For the same reason that distro packages must not run update-desktop-database or update-mime-database or other such commands.

These all create a global cache shared between packages but owned by none. They cannot be owned by a package because then they will have file conflicts with untracked files, or else get overwritten by other packages... that is why you don't just ship the compiled schema directly.

The traditional way to detect that this should not be done, is to only do it when DESTDIR is empty. But existing distro packages are relying on using make install COMPILE_SCHEMAS=0 which is opt in and anyone using that is definitely not going to be surprised that they need to compile the schemas themselves.

Choose either solution, but please restore the ability for distros to package Guake. :)

Oh whoops, thought that was just redundant, opening new pull request with restoration

Thanks! I do understand that maintaining compatibility with distro installs is not always the most obvious or intuitive, so sometimes things slip through the cracks... :)

While you are at it you can go the DESTDIR route, even. ;) It means distros don't need to pass two options when one option will do... Also, it will be more obvious to the next person working on the makefile, that this is special because of distro staged installs rather than just a random option.

Oh, I put in a commit that just reverted, can look at an existence check on DESTDIR

Restoring as per review #1893 (review)

Davidy22 requested a review from mlouielu September 13, 2021 02:17

mlouielu requested changes Sep 13, 2021

View reviewed changes

gsemet requested changes Sep 13, 2021

View reviewed changes

Davidy22 requested review from mlouielu and gsemet September 13, 2021 12:41

gsemet merged commit 93c5cc4 into Guake:master Sep 13, 2021

eli-schwartz reviewed Sep 13, 2021

View reviewed changes

Davidy22 mentioned this pull request Sep 13, 2021

Restore COMPILE_SCHEMA option #1895

Merged

gsemet pushed a commit that referenced this pull request Sep 13, 2021

Restore COMPILE_SCHEMA option

09a958a

Restoring as per review #1893 (review)

Davidy22 mentioned this pull request Sep 30, 2021

Guake won't start due to incorrect SCHEMA_DIR #1436

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix:Regenerate guake schema if old schema file found #1893

fix:Regenerate guake schema if old schema file found #1893

Davidy22 commented Sep 13, 2021

mlouielu left a comment

mlouielu Sep 13, 2021

Davidy22 Sep 13, 2021 •

edited

Loading

mlouielu Sep 13, 2021

mlouielu Sep 13, 2021

Davidy22 Sep 13, 2021

mlouielu Sep 13, 2021

Davidy22 Sep 13, 2021

mlouielu Sep 13, 2021

Davidy22 Sep 13, 2021

mlouielu Sep 13, 2021

Davidy22 Sep 13, 2021

gsemet Sep 13, 2021

Davidy22 Sep 13, 2021

gsemet Sep 13, 2021

gsemet Sep 13, 2021

Davidy22 commented Sep 13, 2021

eli-schwartz Sep 13, 2021 •

edited

Loading

Davidy22 Sep 13, 2021

eli-schwartz Sep 13, 2021 •

edited

Loading

Davidy22 Sep 13, 2021

fix:Regenerate guake schema if old schema file found #1893

fix:Regenerate guake schema if old schema file found #1893

Conversation

Davidy22 commented Sep 13, 2021

mlouielu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidy22 Sep 13, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidy22 commented Sep 13, 2021

eli-schwartz Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidy22 Sep 13, 2021 •

edited

Loading

eli-schwartz Sep 13, 2021 •

edited

Loading

eli-schwartz Sep 13, 2021 •

edited

Loading