| Message ID | 1459351282-16091-1-git-send-email-Vincent.Riera@imgtec.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, On Wed, 30 Mar 2016 16:21:22 +0100, Vicente Olivert Riera wrote: > @@ -32,7 +27,7 @@ HOST_TCL_CONF_OPTS = \ > # I haven't found a good way to force pkgs to not build > # or configure without just removing the entire pkg directory. > define HOST_TCL_REMOVE_PACKAGES > - rm -fr $(@D)/pkgs/sqlite[0-9].[0-9].[0-9] \ > + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ > $(@D)/pkgs/tdbc[0-9].[0-9].[0-9] \ > $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ > $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ Could we switch to using * instead of this silly [0-9] thing? So: rm -rf $(@D)/pkgs/sqlite* $(@D)/pkgs/tdbc* ... > @@ -41,11 +36,10 @@ define HOST_TCL_REMOVE_PACKAGES > endef > HOST_TCL_PRE_CONFIGURE_HOOKS += HOST_TCL_REMOVE_PACKAGES > define TCL_REMOVE_PACKAGES > - rm -fr $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/sqlite[0-9].[0-9].[0-9]) \ > - $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbc[0-9].[0-9].[0-9]) \ > - $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ > + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ > + $(if $(BR2_PACKAGE_MYSQL),,$(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9]) \ > $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ > - $(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9] \ > + $(if $(BR2_PACKAGE_POSTGRESQL),,$(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9]) \ > $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbcsqlite3-[0-9].[0-9].[0-9]) OK, I think this would need a comment above to explain what's going on, it's not trivial to understand why you're removing sqlite and tdbcodbc unconditionally, and tdbcpostgresq/tdbcsqlite conditionally. > endef > TCL_PRE_CONFIGURE_HOOKS += TCL_REMOVE_PACKAGES > @@ -82,6 +76,8 @@ endef > TCL_POST_INSTALL_TARGET_HOOKS += TCL_REMOVE_EXTRA > > TCL_DEPENDENCIES = $(if $(BR2_PACKAGE_SQLITE),sqlite) > +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_MYSQL),mysql) > +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_POSTGRESQL),postgresql) TCL_DEPENDENCIES = \ $(if ...) \ $(if ...) Would be nicer. Maybe this calls for three patches: 1/ Switch to * instead of [0-9] 2/ Fix the sqlite usage 3/ Enable postgresql/mysql usage Thanks! Thomas
Hi Thomas, On 30/03/16 16:35, Thomas Petazzoni wrote: > Hello, > > On Wed, 30 Mar 2016 16:21:22 +0100, Vicente Olivert Riera wrote: > >> @@ -32,7 +27,7 @@ HOST_TCL_CONF_OPTS = \ >> # I haven't found a good way to force pkgs to not build >> # or configure without just removing the entire pkg directory. >> define HOST_TCL_REMOVE_PACKAGES >> - rm -fr $(@D)/pkgs/sqlite[0-9].[0-9].[0-9] \ >> + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ >> $(@D)/pkgs/tdbc[0-9].[0-9].[0-9] \ >> $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ >> $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ > > Could we switch to using * instead of this silly [0-9] thing? So: > > rm -rf $(@D)/pkgs/sqlite* $(@D)/pkgs/tdbc* ... No problem. >> @@ -41,11 +36,10 @@ define HOST_TCL_REMOVE_PACKAGES >> endef >> HOST_TCL_PRE_CONFIGURE_HOOKS += HOST_TCL_REMOVE_PACKAGES >> define TCL_REMOVE_PACKAGES >> - rm -fr $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/sqlite[0-9].[0-9].[0-9]) \ >> - $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbc[0-9].[0-9].[0-9]) \ >> - $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ >> + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ >> + $(if $(BR2_PACKAGE_MYSQL),,$(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9]) \ >> $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ >> - $(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9] \ >> + $(if $(BR2_PACKAGE_POSTGRESQL),,$(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9]) \ >> $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbcsqlite3-[0-9].[0-9].[0-9]) > > OK, I think this would need a comment above to explain what's going on, > it's not trivial to understand why you're removing sqlite and tdbcodbc > unconditionally, and tdbcpostgresq/tdbcsqlite conditionally. We remove sqlite unconditionally because we don't want to use the bundled version. For tdbcodbc I don't have any particular reason to remove it or keep it. What do you prefer? > >> endef >> TCL_PRE_CONFIGURE_HOOKS += TCL_REMOVE_PACKAGES >> @@ -82,6 +76,8 @@ endef >> TCL_POST_INSTALL_TARGET_HOOKS += TCL_REMOVE_EXTRA >> >> TCL_DEPENDENCIES = $(if $(BR2_PACKAGE_SQLITE),sqlite) >> +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_MYSQL),mysql) >> +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_POSTGRESQL),postgresql) > > TCL_DEPENDENCIES = \ > $(if ...) \ > $(if ...) > > Would be nicer. That was my first approach and I didn't look nicer to me, so I changed it :P Anyway, if you prefer that way, I'll do it that way, no problem. > Maybe this calls for three patches: > > 1/ Switch to * instead of [0-9] > 2/ Fix the sqlite usage > 3/ Enable postgresql/mysql usage I agree. Please answer my question about tdbcodbc and I will write those three patches. Regards, Vincent. > Thanks! > > Thomas >
Hello, On Wed, 30 Mar 2016 16:39:24 +0100, Vicente Olivert Riera wrote: > We remove sqlite unconditionally because we don't want to use the > bundled version. Yes, explain this in a comment :) > For tdbcodbc I don't have any particular reason to remove it or keep it. > What do you prefer? If it was already removed before your patch, keep it this way. Thanks! Thomas
Hi Thomas, On 30/03/16 16:41, Thomas Petazzoni wrote: > Hello, > > On Wed, 30 Mar 2016 16:39:24 +0100, Vicente Olivert Riera wrote: > >> We remove sqlite unconditionally because we don't want to use the >> bundled version. > > Yes, explain this in a comment :) Sure. >> For tdbcodbc I don't have any particular reason to remove it or keep it. >> What do you prefer? > > If it was already removed before your patch, keep it this way. It was, yes. Thanks for the quick reply. Regards, Vincent. > > Thanks! > > Thomas >
On 03/30/16 17:39, Vicente Olivert Riera wrote: > Hi Thomas, > > On 30/03/16 16:35, Thomas Petazzoni wrote: >> Hello, >> >> On Wed, 30 Mar 2016 16:21:22 +0100, Vicente Olivert Riera wrote: [snip] >>> TCL_DEPENDENCIES = $(if $(BR2_PACKAGE_SQLITE),sqlite) >>> +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_MYSQL),mysql) >>> +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_POSTGRESQL),postgresql) >> >> TCL_DEPENDENCIES = \ >> $(if ...) \ >> $(if ...) >> >> Would be nicer. > > That was my first approach and I didn't look nicer to me, so I changed > it :P Anyway, if you prefer that way, I'll do it that way, no problem. Like you, Vincent, I also prefer TCL_DEPENDENCIES to be repeated. - You don't need to move your eyes to see which variable is affected. - When adding/removing a dependency, you don't have to worry about the backslashes. I do prefer in this case that all assignments, including the first one, have += Regards, Arnout [snip]
Hello, On Wed, 30 Mar 2016 21:03:27 +0200, Arnout Vandecappelle wrote: > > That was my first approach and I didn't look nicer to me, so I changed > > it :P Anyway, if you prefer that way, I'll do it that way, no problem. > > Like you, Vincent, I also prefer TCL_DEPENDENCIES to be repeated. > > - You don't need to move your eyes to see which variable is affected. > - When adding/removing a dependency, you don't have to worry about the backslashes. > > > I do prefer in this case that all assignments, including the first one, have += Matter of taste, I guess. But if I follow your thought, then we should do: LIBGTK3_CONF_OPTS += --disable-glibtest LIBGTK3_CONF_OPTS += --enable-explicit-deps=no LIBGTK3_CONF_OPTS += --enable-gtk2-dependency LIBGTK3_CONF_OPTS += --disable-introspection I think this is really verbose, but again a matter of taste. Thomas
On 03/30/16 21:06, Thomas Petazzoni wrote: > Hello, > > On Wed, 30 Mar 2016 21:03:27 +0200, Arnout Vandecappelle wrote: > > >> That was my first approach and I didn't look nicer to me, so I changed > >> it :P Anyway, if you prefer that way, I'll do it that way, no problem. > > > > Like you, Vincent, I also prefer TCL_DEPENDENCIES to be repeated. > > > > - You don't need to move your eyes to see which variable is affected. > > - When adding/removing a dependency, you don't have to worry about the > backslashes. > > > > > > I do prefer in this case that all assignments, including the first one, > have += > > Matter of taste, I guess. But if I follow your thought, then we should > do: > > LIBGTK3_CONF_OPTS += --disable-glibtest > LIBGTK3_CONF_OPTS += --enable-explicit-deps=no > LIBGTK3_CONF_OPTS += --enable-gtk2-dependency > LIBGTK3_CONF_OPTS += --disable-introspection > > I think this is really verbose, but again a matter of taste. > Taste is a funny thing... With just plain options I also prefer it backslashified, but when there are $(if ...) clauses it somehow looks cleaner with the +=. Anyway, we have both already and there is no strong reason to choose either. Regards, Arnout
diff --git a/package/tcl/tcl.mk b/package/tcl/tcl.mk index 9fd1195..1ec7741 100644 --- a/package/tcl/tcl.mk +++ b/package/tcl/tcl.mk @@ -14,15 +14,10 @@ TCL_SUBDIR = unix TCL_INSTALL_STAGING = YES TCL_AUTORECONF = YES -# Note that --with-system-sqlite will only make a difference -# in the sqlite package (which gets removed if sqlite not -# configured). Don't need to worry about conditionally including -# it in the configure options TCL_CONF_OPTS = \ --disable-symbols \ --disable-langinfo \ - --disable-framework \ - --with-system-sqlite + --disable-framework HOST_TCL_CONF_OPTS = \ --disable-symbols \ @@ -32,7 +27,7 @@ HOST_TCL_CONF_OPTS = \ # I haven't found a good way to force pkgs to not build # or configure without just removing the entire pkg directory. define HOST_TCL_REMOVE_PACKAGES - rm -fr $(@D)/pkgs/sqlite[0-9].[0-9].[0-9] \ + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ $(@D)/pkgs/tdbc[0-9].[0-9].[0-9] \ $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ @@ -41,11 +36,10 @@ define HOST_TCL_REMOVE_PACKAGES endef HOST_TCL_PRE_CONFIGURE_HOOKS += HOST_TCL_REMOVE_PACKAGES define TCL_REMOVE_PACKAGES - rm -fr $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/sqlite[0-9].[0-9].[0-9]) \ - $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbc[0-9].[0-9].[0-9]) \ - $(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9] \ + rm -fr $(@D)/pkgs/sqlite[0-9].[0-9][0-9].[0-9] \ + $(if $(BR2_PACKAGE_MYSQL),,$(@D)/pkgs/tdbcmysql[0-9].[0-9].[0-9]) \ $(@D)/pkgs/tdbcodbc[0-9].[0-9].[0-9] \ - $(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9] \ + $(if $(BR2_PACKAGE_POSTGRESQL),,$(@D)/pkgs/tdbcpostgres[0-9].[0-9].[0-9]) \ $(if $(BR2_PACKAGE_SQLITE),,$(@D)/pkgs/tdbcsqlite3-[0-9].[0-9].[0-9]) endef TCL_PRE_CONFIGURE_HOOKS += TCL_REMOVE_PACKAGES @@ -82,6 +76,8 @@ endef TCL_POST_INSTALL_TARGET_HOOKS += TCL_REMOVE_EXTRA TCL_DEPENDENCIES = $(if $(BR2_PACKAGE_SQLITE),sqlite) +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_MYSQL),mysql) +TCL_DEPENDENCIES += $(if $(BR2_PACKAGE_POSTGRESQL),postgresql) HOST_TCL_DEPENDENCIES = $(eval $(autotools-package))
- Always remove the bundled sqlite version. - Remove the --with-system-sqlite option because it is an option which only exists in the bundled sqlite configure script. - Always build the basic TDBC. - Build SQLite, MySQL and PostgreSQL TDBC drivers only if these packages have been selected in Buildroot. Add them to TCL_DEPENDENCIES accordingly. Fixes: http://autobuild.buildroot.net/results/022/02296f8624d3406a63d3a179f53862f245c56dc1/ Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com> --- package/tcl/tcl.mk | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)