Skip to content
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

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

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Sep 8, 2024

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 jayaddison marked this pull request as draft September 8, 2024 22:51
@jayaddison
Copy link
Contributor Author

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 jayaddison changed the title Makefile: ensure schema compilation occurs before installation Makefile: ensure schema recompilation occurs after schemas are installed Sep 8, 2024
@jayaddison jayaddison marked this pull request as ready for review September 8, 2024 23:24
@jayaddison
Copy link
Contributor Author

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 jayaddison marked this pull request as draft September 9, 2024 10:42
@jayaddison jayaddison marked this pull request as ready for review September 9, 2024 10:54
@jayaddison
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 jayaddison changed the title Makefile: ensure schema recompilation occurs after schemas are installed Makefile: ensure that schema source files are in place before schema recompilation occurs Sep 14, 2024
Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I was fairly sure the visible change wasn't going to break anything, CI results agree, merging.

@Davidy22 Davidy22 merged commit df9fbbe into Guake:master Sep 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Makefile: race condition between install-schemas and compile-schemas
2 participants