[bug#40994] patch#40994 Programs With Movie Titles (PWMT)

Hi Brice!

> > I don't see a patch related to jumanji, is it voluntary?
> 
> I tried packaging Jumanji, but it is ported to new versions of webkitgtk. We
> do not have older versions of webkitgtk in guix.
> 
> > Could you take example on a commit like
> > aff0cce9175aaf836dd78941eb17549e3bfa7188 (there must be others like it)
> > which move packages to a separate module. In particular create a commit
> > that just move the packages in the new module and adjust the modules 
> > that
> > depend on them (if any). Then add your modifications to the packages in
> > separate commits to the one moving them with the git messages correctly
> > formatted, ie. not in a free form style. Otherwise it's difficult to
> > understand what your package modifications are related to just moving 
> > them
> > elsewhere,since it's just a big diff.
> 
> Thanks for the feedback. Sure thing!
> 
> Please find the attachments with this email, for moving stuffs. All the moved
> packages only refer to each other. So no need of altering any other package
> definitions.
> 
> I will send another set of patches for modifying stuffs,
> 

Here, I have attached another patch-set for modifying stuffs. :-)

Regards,
RG.

Hi!

Raghav Gururajan <raghavgururajan@disroot.org> writes:

> Here, I have attached another patch-set for modifying stuffs. :-)

I have added another set of nit-picks!  :-)

> From 30042d73a58f90971cd6c37bf56269bd3abb2533 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan@disroot.org>
> Date: Sat, 2 May 2020 22:28:44 -0400
> Subject: [PATCH 08/14] gnu: girara: Update package definition.
>
> * gnu/packages/pwmt.scm (girara):
> [source]<origin>[method]: Changed from git-fetch to url-fetch; and
> remove file-name field.
> [arguments]<#:glib-or-gtk?>: New argument.
> [arguments]<#:configure-flags>[-Dnotify]: New flag.
> [native-inputs]<doxygen>: New input.
> [inputs]<glib,gtk+,json-c,libnotify,pango>: New inputs.
> [propagated-inputs]<gtk+>: Removed.
> [synopsis]: Updated.
> [description]: Updated.

I know it's a lot to ask, but it would be great if you could split this
up in multiple patches, one per logical change.  I.e. this one patch
would be better as a series like:

Raghav Gururajan (7):
  gnu: girara: Download tarball instead of git source.
  gnu: girara: Wrap with Glib variables.
  gnu: girara: Add notification support.
  gnu: girara: Build and install documentation.
  gnu: girara: Do not propagate GTK+.
  gnu: girara: Enable more features.
  gnu: girara: Update synopsis & description.

That makes it possible to revert some of these changes in case of
problems without undoing the whole thing, and also makes reviewing
easier.

I'm also skeptical about some of these (why is #:glib-or-gtk? necessary
for this library, why does GTK+ no longer need to be propagated, and
what are all those new inputs for?).  By lumping everything together
it's difficult to reason about these changes.

> From 7e3558dda412d33fffb7bb0668886f1ede3d14c8 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan@disroot.org>
> Date: Sat, 2 May 2020 23:29:28 -0400
> Subject: [PATCH 09/14] gnu: zathura: Update package definition.
>
> * gnu/packages/pwmt.scm (zathura):
> [arguments]<#:glib-or-gtk?>: New argument.
> [native-inputs]<desktop-fileutils,doxygen,python-breathe,python-
> sphinx-rtd-theme>: New inputs.

Same here, what do these inputs do?

> [inputs]<appstream-glib,cairo,file,girara,glib,json-c,gtk+,libnotify,
> libseccomp>: New inputs.

And these?

> [propagated-inputs]<cairo,girara>: Removed.
> [synopsis]: Updated.
> [description]: Updated.

ISTM this could be split into at least four different patches.

> From 345a2b2ffc04c99fdfc3785ac6d19f053afd1b90 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan@disroot.org>
> Date: Sat, 2 May 2020 23:42:49 -0400
> Subject: [PATCH 10/14] gnu: zathura-ps: Update package definition.
>
> * gnu/packages/pwmt.scm (zathura-ps):
> [arguments]<#:glib-or-gtk?>: New argument.

Why does this plugin package need #:glib-or-gtk?.

> [inputs]<cairo,girara,glib,gtk+,json-c,libnotify>: New inputs.

It's strange that all of these packages require almost the exact same
set of inputs.  Perhaps they should be propagated somewhere?

> [synopsis]: Updated.
> [description]: Updated.

This should also be a separate patch.

I think you catch my drift here, can you send an updated series?

Hi Marius!

> I know it's a lot to ask, but it would be great if you could split this
> up in multiple patches, one per logical change.  I.e. this one patch
> would be better as a series like:
> 
> Raghav Gururajan (7):
>   gnu: girara: Download tarball instead of git source.
>   gnu: girara: Wrap with Glib variables.
>   gnu: girara: Add notification support.
>   gnu: girara: Build and install documentation.
>   gnu: girara: Do not propagate GTK+.
>   gnu: girara: Enable more features.
>   gnu: girara: Update synopsis & description.

Oh yeah, this is lot and I cannot do at this time. But I have sent updated
patch-set that splits some changes. :-)

Anyway, this suggestion is very useful, so that I can use it moving forward.

> I'm also skeptical about some of these (why is #:glib-or-gtk? necessary
> for this library, why does GTK+ no longer need to be propagated, and
> what are all those new inputs for?).  By lumping everything together
> it's difficult to reason about these changes.
>
> Same here, what do these inputs do?
> 
> > [inputs]<appstream-glib,cairo,file,girara,glib,json-c,gtk+,libnotify,  
> > libseccomp>: New inputs.  
> 
> And these?

I have used comments in the package definition. :-)

> Why does this plugin package need #:glib-or-gtk?.

That was a mistake. I removed it in the new patch-set.

> > [inputs]<cairo,girara,glib,gtk+,json-c,libnotify>: New inputs.  
> 
> It's strange that all of these packages require almost the exact same
> set of inputs.  Perhaps they should be propagated somewhere?

That's correct. cairo and girara are effect of removing propagation in zathura.
I think it is better not to propagate things, unless it is absolutely
necessary. Other inputs are simply required to generate 'cargs'.

> I think you catch my drift here, can you send an updated series?

I have sent an updated patch-set to the thread, with changes I could do at this
time. :-)

Regards,
RG.