search

Guake / guake

Drop-down terminal for GNOME
https://guake.github.io
GNU General Public License v2.0
4.46k stars 579 forks source link

Makefile: ensure that schema source files are in place before schema recompilation occurs #2257

Closed jayaddison closed 2 months ago

jayaddison commented 2 months ago

When make is invoked in parallel mode (-j parameter), the dependencies of an individual makefile target may be resolved in unpredictable order.

That meant that sometimes, compile-schemas would incorrectly begin to run before all of the gschema XML files had been written to the schema destination directory.

This change relocates the glib-compile-schemas step to ensure that it occurs after the schema files have been written.

In addition, the existing SCHEMA_DIR variable is used instead of gsettingsschemadir to refer to the schema directory.

Resolves #2219

Edit: 20240914: redraft this description for brevity.

jayaddison commented 2 months ago

Ok, my apologies: I definitely should have tested this before opening a pull request.

From attempting the build process locally: it seems that in fact the install_schemas step is intended to occur before compile_schemas -- the latter requires that the files have already been written to the $(SCHEMA_DIR) directory.

I've moved this back into 'draft' pull request status while confirming more of the details.

jayaddison commented 2 months ago

It probably is worth adding a reno entry for this after all.. this race condition can be a source of build nondeterminism, and that means that some downstream packagers might have added -j1 or potentially even infrastructure-level workarounds to prevent guake building in parallel environments. So an informational message in the changelog could help as a hint that it should be OK to remove those workarounds.

jayaddison commented 2 months ago

cc @Davidy22 - I made a bit of a mess developing this PR - my apologies for not testing this until after I opened it - but I believe that it is ready now and resolves a genuine build-time race condition for guake.

Davidy22 commented 2 months ago

Sorry about the delay in checking the build, and seems like github did some deprecating in the time between your testing and me checking this. I could do that seperately and you could pull, or you could bump the version for the upload-artifact action in your tree to v4

jayaddison commented 2 months ago

Thanks for running the CI checks and taking a look! I generally prefer to keep changes like those separate, so if that's OK with you, I'll wait until that is resolved and then will rebase/pull.

Davidy22 commented 2 months ago

Alright, there's still some things to resolve with CI but it should be in a state where you can update your branch and checks here should pass.

jayaddison commented 1 month ago

Somewhat-delayed reply: thank you @Davidy22!