Re: scaffold & gnome-build patch
- From: Gustavo Giráldez <gustavo giraldez gmx net>
- To: aurelien naldi <aurelien resus univ-mrs fr>
- Cc: Gnome Devtools list <gnome-devtools gnome org>
- Subject: Re: scaffold & gnome-build patch
- Date: Thu, 15 Jan 2004 00:30:18 -0300
Hi Aurelien,
On Tue, 2004-01-13 at 14:53, aurelien naldi wrote:
> On mar, 2004-01-13 at 01:46 -0500, John (J5) Palmieri wrote:
> > Sorry, got caught up with work. I just applied your Scaffold patch and
> > it didn't cleanly patch the latest CVS sources. Can you make another
> > patch? I will check out your gnome-build patch tomorow.
>
> the gnome-build one wasn't correct as well, so here they are
>
> I hope those one are OK :)
As before, I'll comment only on the gnome-build patch. Looks good in
general (i.e. the approach is right) but I've got some comments:
> ? Index:
> ? depcomp
> ? missing
> ? mkinstalldirs
> ? src/backends/libgbf_am/PIPO
> Index: src/backends/libgbf_am/gbf-am-build.c
> ===================================================================
> RCS file: /cvs/gnome/gnome-build/src/backends/libgbf_am/gbf-am-build.c,v
> retrieving revision 1.6
> diff -u -r1.6 gbf-am-build.c
> --- src/backends/libgbf_am/gbf-am-build.c 9 Sep 2003 11:11:21 -0000 1.6
> +++ src/backends/libgbf_am/gbf-am-build.c 13 Jan 2004 17:43:07 -0000
> @@ -220,21 +220,17 @@
>
> int
> gbf_build_run (GbfAmProject *project,
> - GbfBuildType type,
> + gchar *id,
> const char *project_dir,
> GList *callbacks)
> {
> static const char *dir_regex = "Entering directory `([^']+)'";
> static const char *warn_regex = "^([^:]+):([0-9]+): warning: (.+)$";
> static const char *err_regex = "^([^:]+):([0-9]+): (.+)$";
> - /* TODO: These paths should come from a build configuration. */
> - static const char *prepare_argv[] = { "./autogen.sh", "--prefix=/gnome", NULL };
> - static const char *configure_argv[] = { "./configure", "--prefix=/gnome", NULL };
> - static const char *clean_argv[] = {"/usr/bin/make", "clean", NULL };
> - static const char *all_argv[] = { "/usr/bin/make", "all", NULL };
> - static const char *install_argv[] = {"/usr/bin/make", "install", NULL };
> +
> static int buildid = 0;
> - const char **argv = NULL;
> + const char *argv[5];
> + const char *build_dir;
These two should not be const, otherwise the compiler warns on the
g_free() later.
> BuildInfo *info;
> char *tmp, *msg;
> int output, err, pid;
> @@ -242,32 +238,53 @@
> reg_syntax_t old_options;
> GError *error = NULL;
> const char *charset;
> + GNode *g_node;
> + GbfAmNode *node;
>
> - switch (type) {
> - case GBF_BUILD_PREPARE:
> - argv = prepare_argv;
> - break;
> - case GBF_BUILD_CONFIGURE:
> - argv = configure_argv;
> - break;
> - case GBF_BUILD_CLEAN:
> - argv = clean_argv;
> - break;
> - case GBF_BUILD_ALL:
> - argv = all_argv;
> - break;
> - case GBF_BUILD_CURRENT:
> - g_warning ("No build for current");
> - break;
> - case GBF_BUILD_INSTALL:
> - argv = install_argv;
> - break;
> - default:
> - g_warning ("Invalid build type");
> + g_node = g_hash_table_lookup(project->targets, id);
> + if (!g_node) {
> + g_warning ("Invalid build: %s", id);
> + return -1;
> + }
Here you should first check whether the passed id is one of the
"special" ones (i.e. clean, all, configure, etc.). Otherwise, get the
node and perform the first part of the if() below.
> +
> + node = GBF_AM_NODE (g_node);
> +
> + if ( 0 == strcmp(node->detail, "program") ||
> + 0 == strcmp(node->detail, "static_lib") ||
> + 0 == strcmp(node->detail, "shared_lib") ) {
> + /* find the right build dir and make argument */
> + /* FIXME: durty ?? */
> + gchar *cur, *last;
> + cur = id;
> + while( *cur != '\0' && *cur != ':') {
> + if ('/' == *cur) {
> + last = cur;
> + }
> + cur++;
> + }
> + *last = '\0';
> + *cur = '\0';
> + build_dir = g_strdup_printf("%s%s", project_dir, id);
This isn't nice, but I know there's no other way to do it now... We'll
have to fix it later.
> +
> + argv[0] = g_strdup(project->make_command);
> + argv[1] = g_strdup(last+1);
> + argv[2] = NULL;
> + } else if ( 0 == strcmp(node->detail, "configure") ) {
As stated above, you need to strcmp() the id, not the node, and move
these conditions before retrieving the project node.
> + argv[0] = g_strdup(project->configure_command);
> + argv[1] = g_strdup_printf("--prefix=%s",project->install_prefix);
> + argv[2] = NULL;
> + build_dir = g_strdup(project_dir);
> + } else if (0 == strcmp(node->detail, "autogen")) {
> + argv[0] = g_strdup(project->autogen_command);
> + argv[1] = g_strdup_printf("--prefix=%s",project->install_prefix);
> + argv[2] = NULL;
> + build_dir = g_strdup(project_dir);
> + } else {
> + g_warning ("Invalid build type : %s", node->detail);
> return -1;
> }
>
> - if (!g_spawn_async_with_pipes (project_dir,
> + if (!g_spawn_async_with_pipes (build_dir,
> (char**)argv, NULL,
> 0,
> NULL, NULL,
> @@ -275,8 +292,14 @@
> NULL, &output, &err,
> NULL)) {
> g_warning ("Couldn't spawn '%s'", argv[0]);
> + g_free(build_dir);
> + g_free(argv[0]);
> + g_free(argv[1]);
> return -1;
> }
> + g_free(build_dir);
> + g_free(argv[0]);
> + g_free(argv[1]);
>
> out_channel = g_io_channel_unix_new (output);
> g_io_channel_set_close_on_unref (out_channel, TRUE);
> @@ -297,7 +320,6 @@
>
> info = g_new0 (BuildInfo, 1);
> info->project = project;
> - info->type = type;
> info->id = ++buildid;
> info->num_channels = 2;
> info->callbacks = callbacks;
> Index: src/backends/libgbf_am/gbf-am-build.h
> ===================================================================
> RCS file: /cvs/gnome/gnome-build/src/backends/libgbf_am/gbf-am-build.h,v
> retrieving revision 1.1
> diff -u -r1.1 gbf-am-build.h
> --- src/backends/libgbf_am/gbf-am-build.h 18 Sep 2002 16:38:45 -0000 1.1
> +++ src/backends/libgbf_am/gbf-am-build.h 13 Jan 2004 17:43:07 -0000
> @@ -28,7 +28,7 @@
> G_BEGIN_DECLS
>
> int gbf_build_run (GbfAmProject *project,
> - GbfBuildType type,
> + gchar *id,
> const char *project_dir,
> GList *callbacks);
>
> Index: src/backends/libgbf_am/gbf-am-project.c
> ===================================================================
> RCS file: /cvs/gnome/gnome-build/src/backends/libgbf_am/gbf-am-project.c,v
> retrieving revision 1.47
> diff -u -r1.47 gbf-am-project.c
> --- src/backends/libgbf_am/gbf-am-project.c 14 Sep 2003 17:27:16 -0000 1.47
> +++ src/backends/libgbf_am/gbf-am-project.c 13 Jan 2004 17:43:07 -0000
> @@ -71,7 +71,7 @@
> typedef struct {
> GbfAmProject *project;
> GbfAmProjectOpType type;
> - GbfBuildType build_type;
> + gchar *build_id;
> } GbfAmProjectOp;
>
>
> @@ -171,7 +171,11 @@
>
> enum {
> PROP_0,
> - PROP_PROJECT_DIR
> + PROP_PROJECT_DIR,
> + PROP_PROJECT_MAKE_COMMAND,
> + PROP_PROJECT_CONFIGURE_COMMAND,
> + PROP_PROJECT_AUTOGEN_COMMAND,
> + PROP_PROJECT_INSTALL_PREFIX
> };
Here you seem to want to add the command strings as GObject properties
but you don't install them later (using g_object_install_property()).
In any case, they are not necessary (or useful) since they can't be
exposed as it'd break the abstraction of GbfProject.
> static GbfProject *parent_class;
> @@ -303,7 +307,7 @@
> gchar *project_root = uri_to_path (project->project_root_uri);
>
> /* FIXME: gbf_build_run can be simpler */
> - gbf_build_run (project, op->build_type,
> + gbf_build_run (project, op->build_id,
> project_root,
> project->callbacks);
>
> @@ -2341,11 +2345,15 @@
> project = GBF_AM_PROJECT (_project);
> op = g_new0 (GbfAmProjectOp, 1);
> op->project = project;
> - // FIXME
> op->type = BUILD;
> - //op->build_type = type;
>
> - queue_push_op (project, op);
> + /* FIXME this ain't nice, works for now ? */
> + g_hash_table_lookup (project->groups, id);
> + if (g_hash_table_lookup (project->targets, id))
> + {
> + op->build_id = id;
> + queue_push_op (project, op);
> + }
I'd suggest you remove the checks here and just queue the operation.
While it's not correct, you'd be required to check for the special build
targets here also.
Also, you need to g_strdup() the id (and g_free() it when the op is
popped of course).
> }
>
> static void
> @@ -2353,10 +2361,17 @@
> GError **error)
> {
> GbfAmProject *project;
> + GbfAmProjectOp *op;
>
> g_return_if_fail (GBF_IS_AM_PROJECT (_project));
>
> project = GBF_AM_PROJECT (_project);
> + op = g_new0 (GbfAmProjectOp, 1);
> + op->project = project;
> + op->type = BUILD;
> + op->build_id = "clean";
g_strdup() the id here too.
> +
> + queue_push_op (project, op);
> }
>
> static gboolean
> @@ -2411,6 +2426,29 @@
> g_signal_emit_by_name (G_OBJECT (project), "project-updated");
> }
>
> +
> +static void
> +foreach_build_target (gpointer key, gpointer value, gpointer data)
> +{
> + GList **targets = data;
> + GbfAmNode *node;
> + GbfBuildTarget *target;
> +
> + node = GBF_AM_NODE ((GNode *)value);
> +
> + if ( 0 == strcmp(node->detail, "program") ||
> + 0 == strcmp(node->detail, "static_lib") ||
> + 0 == strcmp(node->detail, "shared_lib") ) {
> + target = g_new0 (GbfBuildTarget, 1);
> + target->id = g_strdup (key);
Here it would be interesting to add some prefix to the id to indicate
it's a project target being built. Otherwise you couldn't tell the
difference between the special "configure" target and a "configure"
program.
Adding a "target:" prefix (or similar) should be enough. Of course,
you'll need to change gbf_build_run() accordingly.
> + target->label = g_strdup (node->name);
> + target->description = g_strdup_printf("Build specific target: %s", node->name);
This string needs to be marked up for translation.
> +
> + *targets = g_list_append (*targets, target);
> + }
> +}
> +
> +
> static GList *
> impl_get_build_targets (GbfProject *_project,
> GError **error)
> @@ -2438,6 +2476,9 @@
> target->description = g_strdup (_("Build the entire project and install it"));
> targets = g_list_append (targets, target);
>
> + /* other build targets */
> + g_hash_table_foreach (project->targets, foreach_build_target, &targets);
> +
> return targets;
> }
>
> @@ -3173,6 +3214,12 @@
>
> /* initialize build callbacks */
> project->callbacks = NULL;
> +
> + /* FIXME: those path should be configurable */
> + project->make_command = g_strdup("/usr/bin/make");
> + project->configure_command = g_strdup("./configure");
> + project->autogen_command = g_strdup("./autogen");
> + project->install_prefix = g_strdup("/gnome");
> }
>
> static void
> @@ -3195,7 +3242,11 @@
>
> /* disconnect callbacks */
> callbacks_destroy (project);
> -
> + g_free(project->make_command);
> + g_free(project->configure_command);
> + g_free(project->autogen_command);
> + g_free(project->install_prefix);
> +
> GNOME_CALL_PARENT (G_OBJECT_CLASS, dispose, (object));
> }
>
> @@ -3211,6 +3262,18 @@
> case PROP_PROJECT_DIR:
> g_value_set_string (value, project->project_root_uri);
> break;
> + case PROP_PROJECT_MAKE_COMMAND:
> + g_value_set_string (value, project->make_command);
> + break;
> + case PROP_PROJECT_CONFIGURE_COMMAND:
> + g_value_set_string (value, project->configure_command);
> + break;
> + case PROP_PROJECT_AUTOGEN_COMMAND:
> + g_value_set_string (value, project->autogen_command);
> + break;
> + case PROP_PROJECT_INSTALL_PREFIX:
> + g_value_set_string (value, project->install_prefix);
> + break;
These should not go here. See my comment above.
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> break;
> Index: src/backends/libgbf_am/gbf-am-project.h
> ===================================================================
> RCS file: /cvs/gnome/gnome-build/src/backends/libgbf_am/gbf-am-project.h,v
> retrieving revision 1.14
> diff -u -r1.14 gbf-am-project.h
> --- src/backends/libgbf_am/gbf-am-project.h 26 Apr 2003 09:31:02 -0000 1.14
> +++ src/backends/libgbf_am/gbf-am-project.h 13 Jan 2004 17:43:07 -0000
> @@ -91,6 +91,12 @@
>
> /* build callbacks */
> GList *callbacks;
> +
> + /* build config */
> + gchar *make_command;
> + gchar *configure_command;
> + gchar *autogen_command;
> + gchar *install_prefix;
> };
>
> struct _GbfAmProjectClass {
> Index: src/gbf/gbf-project-view.c
> ===================================================================
> RCS file: /cvs/gnome/gnome-build/src/gbf/gbf-project-view.c,v
> retrieving revision 1.5
> diff -u -r1.5 gbf-project-view.c
> --- src/gbf/gbf-project-view.c 26 Dec 2003 05:30:35 -0000 1.5
> +++ src/gbf/gbf-project-view.c 13 Jan 2004 17:43:07 -0000
> @@ -323,7 +323,7 @@
> GBF_PROJECT_MODEL_COLUMN_DATA, &data,
> -1);
> /* walk up the hierarchy searching for a node of the given type */
> - while (data->type != type) {
> + while (NULL != data && data->type != type) {
> gbf_tree_data_free (data);
> data = NULL;
>
> @@ -333,6 +333,7 @@
> gtk_tree_model_get (model, &iter2,
> GBF_PROJECT_MODEL_COLUMN_DATA, &data,
> -1);
> + iter = iter2;
Doh :-) Nice catch.
The glue-factory.c and glue-plugin.c hunks are already committed (I
fixed the warnings yesterday).
Minor nit pick: Please leave a space between the opening parenthesis and
the last character. I notice you don't do that in some places.
Thanks for your work.
Regards,
Gustavo
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
