Re: [PATCH] gnu: Add vim-full.

[ng0 <ng0@we.make.ritual.n0.is>]

Hi,

thanks for reviewing.

Marius Bakke <mbakke@fastmail.com> writes:

> ng0 <ng0@we.make.ritual.n0.is> writes:
>
>> * gnu/packages/vim.scm (vim-full): New variable.
>> * gnu/packages/patches/vim-8.0.0003.patch: New file.
>> * gnu/packages/patches/vim-8.0.0004.patch: New file.
>> * gnu/packages/patches/vim-8.0.0005.patch: New file.
>> * gnu/local.mk (dist_patch_DATA): Add patches.
>> ---
>>  gnu/local.mk                            |  3 ++
>>  gnu/packages/patches/vim-8.0.0003.patch | 87 +++++++++++++++++++++++++++++++++
>>  gnu/packages/patches/vim-8.0.0004.patch | 60 +++++++++++++++++++++++
>>  gnu/packages/patches/vim-8.0.0005.patch | 45 +++++++++++++++++
>>  gnu/packages/vim.scm                    | 81 ++++++++++++++++++++++++++++++
>>  5 files changed, 276 insertions(+)
>>  create mode 100644 gnu/packages/patches/vim-8.0.0003.patch
>>  create mode 100644 gnu/packages/patches/vim-8.0.0004.patch
>>  create mode 100644 gnu/packages/patches/vim-8.0.0005.patch
>
> [...]
>
>> +
>> +(define-public vim-full
>> +  (package
>> +    (inherit vim)
>> +    (name "vim-full")
>> +    (version (package-version vim))
>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri (string-append "ftp://ftp.vim.org/pub/vim/unix/vim-"
>> +                           version ".tar.bz2"))
>> +       (sha256
>> +        (base32
>> +         "1s34rf8089klsbdx5l0iw7vjymir0kzfrx8wb30s31wygnq29axc"))
>> +       ;; Patches need to be applied sequentially. 8.0 is the release of
>> +       ;; vim version 8.0.0002 so we start at 8.0.0003
>> +       (patches (search-patches "vim-8.0.0003.patch"
>> +                                "vim-8.0.0004.patch"
>> +                                "vim-8.0.0005.patch"))))
>
> Shouldn't these patches be applied on the base vim package? Then
> "vim-full" can just inherit the source from there. I also think
> "version" should be adjusted to match the patch level.

No (for explanation see the other emails I've sent in the earlier
thread).
I agree on the version, I will update that.

> Other than that this package works for me. Perhaps the description could
> be updated to mention that this also includes the graphical "gvim" if
> anyone is searching for that.

Okay, I will include that.

> Could you separate this out to two patches, one that updates "vim" and
> one that adds "vim-full"?

See above. But I think hardly anyone will search for it, so here's the
reason: The patches are only needed for what vim-full does and for that
only to make the tests succeed. I don't see any rational point in
including patches which will make a testsuite succeed which is not run
by the package you suggest I apply them to.
If this is extended in the future (say vim fixes some serious bug where
we need to sequentially apply the patches up to that), feel free to
change it to vim. I just need to make the testsuite succeed, which the
patches even comment afaik.
If it would extend any functionality our standard (minimal) vim has, I
would apply them, but as far as I see it this is not the case.
Correct me if I'm wrong.

> Thanks!
>