Re: 04/05: gnu: swig: Patch for Octave 4.4.

[Mark H Weaver <mhw@netris.org>]

Hi Kei,

Kei Kebreau <kkebreau@posteo.net> writes:

> Mark H Weaver <mhw@netris.org> writes:
>
>> Instead, I suggest creating a different 'swig' package for octave that
>> inherits from the primary one and adds this patch.
>
> I agree that this will work. Is the attached patch something like what
> you're thinking?

Yes, although I have some suggestions:

> From 475a7698fd4c88ea688cb43499344e65ffaf5b84 Mon Sep 17 00:00:00 2001
> From: Kei Kebreau <kkebreau@posteo.net>
> Date: Wed, 30 May 2018 08:34:42 -0400
> Subject: [PATCH] gnu: shogun: Use a patched swig for Octave 4.4.
>
> * gnu/packages/machine-learning.scm (swig-for-octave): New variable
> * gnu/packages/machine-learning.scm (shogun)[inputs]: Replace swig with
> swig-for-octave.
> * gnu/packages/patches/swig-octave-patches.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Register patch.
[...]
> diff --git a/gnu/packages/machine-learning.scm b/gnu/packages/machine-learning.scm
> index e135ee0ee..4d54584a7 100644
> --- a/gnu/packages/machine-learning.scm
> +++ b/gnu/packages/machine-learning.scm
> @@ -365,6 +365,14 @@ value imputation, classifier creation, generalization error estimation and
>  sample proximities between pairs of cases.")
>      (license license:gpl3+)))
>  
> +(define swig-for-octave
> +  (package/inherit swig
> +    (name (string-append (package-name swig) "-for-octave"))
> +    (source
> +     (origin
> +       (inherit (package-source swig))
> +       (patches (search-patches "swig-octave-patches.patch"))))))

For the patches field, how about something like this:

   (patches (append (origin-patches (package-source swig))
                    (search-patches "swig-octave-patches.patch")))

so if someone adds a patch to swig, it will automatically get picked up
by this one too.

Please add a comment above the 'swig-for-octave' definition explaining
what it's for.  I was confused at first that you didn't change the
octave definition, but now I guess that it's for other packages that
wish to use octave via swig, or something along those lines?  It should
be good to explain in a comment.

Also, I think it would be better to put the 'swig-for-octave' definition
immediately after the one for 'swig', so that it's more likely to be
noticed by people working on swig.  This package has more connection to
swig than it does to shogun.  It has no direct connection to 'shogun'
except that shogun is currently the only user of it.

Another question: instead of adding the patch, how about changing the
origin to build from a git checkout?  The advantage of that is that it
requires less trust from users.  I look at this big patch and I wonder
if I should check to make sure it matches what upstream has (I didn't).
If it's a git checkout, there's less trust needed.  It would also make
later upgrades easier.

It might also be better to call the package "swig-git" or something,
dunno.

What do you think?  Thanks for working on it.

      Mark