[bug#39384] gnu: Add emacs-rg.

Thank you for the fast feedback!

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
> Nitpick: I think the trend is to align `base32' with the string.

> You may want to lint your package. In particular, the synopsis 
> should be
> akin to "Search tool based ..."

> The description must start with a full sentence, e.g., "rg.el" 
> is an
> Emacs search package...

> Texinfo requires two spaces after the full stop.

> Ditto. Besides, the quote after "users" looks suspicious. You 
> should use
> a regular quote.

A patch file is attached that addresses all of the above feedback. 
The output of `guix lint emacs-rg` is now clean on my system; 
thank you for making me aware of that utility.

The only part of the package I'm uncertain about is declaring 
ripgrep as a propagated dependency.  ripgrep is not needed for 
this Emacs package to be able to byte-compile successfully, but 
`rg` does not need to be on PATH for the package to be useful at 
all.  So while I imagine the majority of the uses-cases would want 
to have ripgrep installed locally, it's definitely plausible that 
one could only ever want to use emacs-rg via TRAMP in which case 
pulling in ripgrep would be completely unnecessary.

Please let me know what you think.

--
Joseph LaFreniere

On Sun, Feb 02, 2020 at 08:21:52AM -0600, LaFreniere, Joseph wrote:
> Thank you for the fast feedback!
> 
> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
> > Nitpick: I think the trend is to align `base32' with the string.
> 
> > You may want to lint your package. In particular, the synopsis should be
> > akin to "Search tool based ..."
> 
> > The description must start with a full sentence, e.g., "rg.el" is an
> > Emacs search package...
> 
> > Texinfo requires two spaces after the full stop.
> 
> > Ditto. Besides, the quote after "users" looks suspicious. You should use
> > a regular quote.
> 
> A patch file is attached that addresses all of the above feedback. The
> output of `guix lint emacs-rg` is now clean on my system; thank you for
> making me aware of that utility.
> 
> The only part of the package I'm uncertain about is declaring ripgrep as a
> propagated dependency.  ripgrep is not needed for this Emacs package to be
> able to byte-compile successfully, but `rg` does not need to be on PATH for
> the package to be useful at all.  So while I imagine the majority of the
> uses-cases would want to have ripgrep installed locally, it's definitely
> plausible that one could only ever want to use emacs-rg via TRAMP in which
> case pulling in ripgrep would be completely unnecessary.
> 
> Please let me know what you think.

Is it possible to patch the invocations of `rg` to refer to ripgrep
directly?

Efraim Flashner <efraim@flashner.co.il> writes:
> Is it possible to patch the invocations of `rg` to refer to 
> ripgrep directly?

Thank you for the input, but I don't understand what you mean by 
"referring to ripgrep directly".  Can you elaborate on that 
proposal and how it would help resolve the question of whether to 
include ripgrep as a propagated input?

--
Joseph LaFreniere

On Sun, Feb 02, 2020 at 09:41:41PM -0600, LaFreniere, Joseph wrote:
> 
> Efraim Flashner <efraim@flashner.co.il> writes:
> > Is it possible to patch the invocations of `rg` to refer to ripgrep
> > directly?
> 
> Thank you for the input, but I don't understand what you mean by "referring
> to ripgrep directly".  Can you elaborate on that proposal and how it would
> help resolve the question of whether to include ripgrep as a propagated
> input?

We want to have ripgrep as a regular input, so at the point in the code
where it searches through PATH for the rg binary we patch it to refer to
the specific binary. One example of this is in (gnu packages emacs-xyz),
with emacs-nov-el.

Efraim Flashner <efraim@flashner.co.il> writes:
> We want to have ripgrep as a regular input, so at the point in 
> the code
> where it searches through PATH for the rg binary we patch it to 
> refer to
> the specific binary. One example of this is in (gnu packages 
> emacs-xyz),
> with emacs-nov-el.

Ah, I see what you mean now.  But wouldn't hard-coding the path to 
ripgrep in that way prevent the package from being able to use 
remote systems' ripgrep binaries when running over TRAMP?

--
Joseph LaFreniere

On February 5, 2020 3:08:11 AM UTC, "LaFreniere, Joseph" <joseph@lafreniere.xyz> wrote:
>
>Efraim Flashner <efraim@flashner.co.il> writes:
>> We want to have ripgrep as a regular input, so at the point in 
>> the code
>> where it searches through PATH for the rg binary we patch it to 
>> refer to
>> the specific binary. One example of this is in (gnu packages 
>> emacs-xyz),
>> with emacs-nov-el.
>
>Ah, I see what you mean now.  But wouldn't hard-coding the path to 
>ripgrep in that way prevent the package from being able to use 
>remote systems' ripgrep binaries when running over TRAMP?
>

Unfortunately that's not something I know. I remember that for the tramp package itself there were some changes related to PATH to make it work when connecting between a Guix System and a foreign one. Hopefully someone knows the answer.

"LaFreniere\, Joseph" <joseph@lafreniere.xyz> writes:

> Efraim Flashner <efraim@flashner.co.il> writes:
>> We want to have ripgrep as a regular input, so at the point in 
>> the code
>> where it searches through PATH for the rg binary we patch it to 
>> refer to
>> the specific binary. One example of this is in (gnu packages 
>> emacs-xyz),
>> with emacs-nov-el.
>
> Ah, I see what you mean now.  But wouldn't hard-coding the path to 
> ripgrep in that way prevent the package from being able to use 
> remote systems' ripgrep binaries when running over TRAMP?

Perhaps we could patch it to do both?  Use the store prefix if it
exists, and fall back to searching in PATH?

Marius Bakke <mbakke@fastmail.com> writes:
> "LaFreniere\, Joseph" <joseph@lafreniere.xyz> writes:
>> Ah, I see what you mean now.  But wouldn't hard-coding the path 
>> to
>> ripgrep in that way prevent the package from being able to use
>> remote systems' ripgrep binaries when running over TRAMP?
>
> Perhaps we could patch [emacs-rg] to do both?  Use the store 
> prefix if it
> exists, and fall back to searching in PATH?

What would be the advantage of that over just searching PATH to 
start with?

--
Joseph LaFreniere

On Thu, Feb 06, 2020 at 06:47:23PM -0600, LaFreniere, Joseph wrote:
> 
> Marius Bakke <mbakke@fastmail.com> writes:
> > "LaFreniere\, Joseph" <joseph@lafreniere.xyz> writes:
> > > Ah, I see what you mean now.  But wouldn't hard-coding the path to
> > > ripgrep in that way prevent the package from being able to use
> > > remote systems' ripgrep binaries when running over TRAMP?
> > 
> > Perhaps we could patch [emacs-rg] to do both?  Use the store prefix if
> > it
> > exists, and fall back to searching in PATH?
> 
> What would be the advantage of that over just searching PATH to start with?

It will still work even if you don't have ripgrep specifically
installed.

Efraim Flashner <efraim@flashner.co.il> writes:
> On Thu, Feb 06, 2020 at 06:47:23PM -0600, LaFreniere, Joseph 
> wrote:
>>
>> Marius Bakke <mbakke@fastmail.com> writes:
>> > "LaFreniere\, Joseph" <joseph@lafreniere.xyz> writes:
>> > > Ah, I see what you mean now.  But wouldn't hard-coding the 
>> > > path to
>> > > ripgrep in that way prevent the package from being able to 
>> > > use
>> > > remote systems' ripgrep binaries when running over TRAMP?
>> >
>> > Perhaps we could patch [emacs-rg] to do both?  Use the store 
>> > prefix if
>> > it
>> > exists, and fall back to searching in PATH?
>>
>> What would be the advantage of that over just searching PATH to 
>> start with?
>
> It will still work even if you don't have ripgrep specifically
> installed.

Can you point me to the Guix documentation where the functionality 
you're describing is explained?  I have read through the 
description of package inputs in section 6.2.1 of Guix's manual, 
but I still do not explain what advantage patching the search path 
offers.

My understanding is that if we want to preserve both local and 
remote-via-TRAMP functionality, we can either
- just include ripgrep as a propagated input, or
- include ripgrep as a propagated input _and_ patch the package to 
  look for ripgrep in a hardcoded location (for local) as well as 
  PATH (for TRAMP).

Both options would have ripgrep included as propagated input.  As 
soon as ripgrep is installed in a user's profile, its binary will 
be available on PATH.  If that is correct, then I don't see any 
advantage to patching in a hardcoded path to ripgrep.

--
Joseph LaFreniere

On Sat, Feb 08, 2020 at 04:23:08PM -0600, LaFreniere, Joseph wrote:
> 
> Efraim Flashner <efraim@flashner.co.il> writes:
> > On Thu, Feb 06, 2020 at 06:47:23PM -0600, LaFreniere, Joseph wrote:
> > > 
> > > Marius Bakke <mbakke@fastmail.com> writes:
> > > > "LaFreniere\, Joseph" <joseph@lafreniere.xyz> writes:
> > > > > Ah, I see what you mean now.  But wouldn't hard-coding the > >
> > > path to
> > > > > ripgrep in that way prevent the package from being able to > >
> > > use
> > > > > remote systems' ripgrep binaries when running over TRAMP?
> > > >
> > > > Perhaps we could patch [emacs-rg] to do both?  Use the store >
> > > prefix if
> > > > it
> > > > exists, and fall back to searching in PATH?
> > > 
> > > What would be the advantage of that over just searching PATH to
> > > start with?
> > 
> > It will still work even if you don't have ripgrep specifically
> > installed.
> 
> Can you point me to the Guix documentation where the functionality you're
> describing is explained?  I have read through the description of package
> inputs in section 6.2.1 of Guix's manual, but I still do not explain what
> advantage patching the search path offers.

I'm not sure I can find a spot in the manual where it is detailed. It
comes down to the difference between "search for this program in PATH"
and "call this program located at this location". By calling the rg
at it's exact path rg doesn't need to be installed directly.

> My understanding is that if we want to preserve both local and
> remote-via-TRAMP functionality, we can either
> - just include ripgrep as a propagated input, or
> - include ripgrep as a propagated input _and_ patch the package to  look for
> ripgrep in a hardcoded location (for local) as well as  PATH (for TRAMP).

The second option is to include ripgrep as an input and patch the
package to look for it at a hardcoded location (for local) as well as
PATH (for TRAMP).

> Both options would have ripgrep included as propagated input.  As soon as
> ripgrep is installed in a user's profile, its binary will be available on
> PATH.  If that is correct, then I don't see any advantage to patching in a
> hardcoded path to ripgrep.
> 
> --
> Joseph LaFreniere