Re: opinion needed on feature/download

Hello Mat

On 23 November 2012 19:06, Mat M <matm@gmx.fr> wrote:
> Hello Matus,
>
> I plannned to answer in the issue, but I saw your comment about the ML,so...

You could have.. I just wanted to let people know here what I am
working on to be sure nobody will be strongly against (maybe it was
pointless)
I think it's ~done now and I will probably just push it. (with
reverted last commit)

> So now, with your current implementation:
> For one tarball foobar-1.1.4.tar.gz we do:
> - Specify FOOBAR_TARBALL :=
> "48d647fbd8ef8889e5a7f422c1bfda94-foobar-1.1.4.tar.gz" in download.lst
> - Specify $(call fetch_Optional,FOOBAR,$(FOOBAR_TARBALL)) \ to depending url
> call in Makefile.fetch

that's right

> - Add a block to test for the need of the tarball in configure.ac
> if test "$need_foobar" = "yes"; then
>     BUILD_TYPE="$BUILD_TYPE FOOBAR"
> fi

Not really.
We use BUILD_TYPE to determine which modules to build (although this
information is often redundant with SYSTEM_FOO)

> What i do not like:
> - "too much" machinery in makefile.fetch

Indeed, but there is nothing to do about it I am afraid.

> You moved in commit
> f9557d7f82648febe40067fa69e96525d938c16b [1] two conditionals from
> configure.ac into it (mozab & libxmlsec). They should go back and TARBALL
> variables replaced with BUILD_TYPE, IMO.

But their names now must be defined in download.lst.
For mozilla stuff I've used MOZ, for libxmlsec you could use DESKTOP
but it will be needed for build platform anyway (and downloaded)

> If we put some logic in make fetch
> we are doomed to forget/ not know it and kept trapped into
> why-it-does-not-download-although-my-configure-is-ok.

Well, but Makefile.fetch is the first (second) place to look into if
you are trying to answer "why-it-does-not-get-downloaded" question, so
it should be fine ?

> Improvement:
> Since you check for wget or curl in configure, I would set one variable

This would make configure more complicated, I did not want to do that.
IMHO configure is just for very basic stuff and exporting variable
name for command sounds fine I think.

> FETCHTAR that will be used in fetch_Download_get_command instead of having
> if'ed define (BTW, why 2 underscores in function name ?)

2 underscored are just a habit from gbuild.
We use <namespace>_<class>_<'public'-method> or
<namespace>_<class>__<'private'-method> defines in gbuild.
Example gb_UnoApiTarget_add_idlfile vs. gb_UnoApiTarget__add_idlfile
in solenv/gbuild/UnoApiTarget.mk

Thanks for your careful review, it's really cool.

All the best,
Matus