fix:Regenerate guake schema if old schema file found - #1893
Conversation
mlouielu
left a comment
mlouielu
left a comment
There was a problem hiding this comment.
Some points I want to know more about it.
| 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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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: |
There was a problem hiding this comment.
Prefer to use pathlib (as we only support >= 3.6)
with open(pathlib.Path(SCHEMA_DIR / "version"), ...)
There was a problem hiding this comment.
Wasn't os.path.join already around before 3.6? I'll change it to pathlib anyways
There was a problem hiding this comment.
yep, but pathlib will handle any annoying file system stuff (although Guake didn't run on different OS...)
There was a problem hiding this comment.
Changed to pathlib
| 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: |
There was a problem hiding this comment.
Prefer pathlib, and I don't think we need encoding="utf-8", they won't contain unicode codepoint character.
There was a problem hiding this comment.
Including encoding in open() calls makes the linter happy. I'll change it to ascii.
There was a problem hiding this comment.
I changed the explicit encoding to ascii already. Probably don't want to remove the check from our linter, it's an alright check
| 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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Didn't consider using a param in the schema file. Changed approach to use that. No additional version tracking files now.
| 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()) |
There was a problem hiding this comment.
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.
|
Changed the approach somewhat to use a setting in the schema file. |
|
|
||
| compile-shemas: | ||
| if [ $(COMPILE_SCHEMA) = 1 ]; then glib-compile-schemas $(DESTDIR)$(gsettingsschemadir); fi | ||
| glib-compile-schemas $(DESTDIR)$(gsettingsschemadir) |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
Oh whoops, thought that was just redundant, opening new pull request with restoration
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I put in a commit that just reverted, can look at an existence check on DESTDIR
Restoring as per review #1893 (review)
4 participants
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.