GNOME Bugzilla – Bug 782426
Use breadth-first for flatpak manifest discover
Last modified: 2017-07-07 21:55:08 UTC
Currently Builder uses deep-first, that makes manifest deep in the hierarchy being pick up first rather than manifest in the root folder. It would be ideal to have a breadth-first enumeration, so manifes closer to the root are pickp up first. Of course the ideal solution for handling manifest would be different, but it's a nice touch that we can fix soon. Relevant code is in https://git.gnome.org/browse/gnome-builder/tree/plugins/flatpak/gbp-flatpak-configuration-provider.c#n786
Task for newcomers, it's a nice one to get started with Builder contribution.
The GbpFlatpakBuildSystemDiscover already does this, so that code can probably be copied into the configuration provider. https://git.gnome.org/browse/gnome-builder/tree/plugins/flatpak/gbp-flatpak-build-system-discovery.c#n38
Created attachment 354774 [details] [review] Use breadth-first in flatpak discovery My first contribution. I changed the behaviour to breadth-first. The code is similiar to that of build-system-discovery so maybe we should refactor this out in a utility function.
Review of attachment 354774 [details] [review]: Nice working putting your first GNOME Builder patch together! Couple quick things to address and then we can merge. ::: plugins/flatpak/gbp-flatpak-configuration-provider.c @@ -867,3 +871,5 @@ - else if (!gbp_flatpak_configuration_provider_find_manifests (self, file, configs, cancellable, error)) - return FALSE; - continue; + + if (depth < DISCOVERY_MAX_DEPTH - 1) + { ... 2 more ... Only 2 spaces for indentation here. @@ +926,3 @@ + return FALSE; + + return gbp_flatpak_configuration_provider_find_manifests (self, file, configs, depth + 1, cancellable, error); You probably don't want to unconditionally return from the first child directory check. So probably something to the tune of: if (find_manifests(...)) return TRUE;
Review of attachment 354774 [details] [review]: ::: plugins/flatpak/gbp-flatpak-configuration-provider.c @@ +926,3 @@ + return FALSE; + + return gbp_flatpak_configuration_provider_find_manifests (self, file, configs, depth + 1, cancellable, error); Or rather, if (!find_manifests(...)) since that is the error case. Although, you might just want to ignore errors altogether from the child directories.
Wouldn't it be easier to check in gbp_flatpak_configuration_provider_load_worker for an empty error instead to rely on the return boolean? So we would catch an error in every case and we could change it to an void function.
Switching various functions to void and keeping the GError in the local scope seems reasonable to me.
Created attachment 354970 [details] [review] overworked patch for breadth-first Changed it according to Comment #6. I check now for error != null instead to check for boolean values. The only thing which i am not sure is when we cancelled. Should we check this state independently? (i think error is null even if it got cancelled - so this state wasn't checked before this change)
Review of attachment 354970 [details] [review]: Hrmm, it looks like maybe some things are missing from this patch based on the comment?
Created attachment 355043 [details] [review] correct overworked patch for breadth-first Sorry this should be now the correct patch - i somehow mixed the patchfiles up
Well done! Thanks! I've also added a followup commit to add you to AUTHORS and our about dialog credits.