[bug#54304] Don't fix git executable location during Elixir build

The removed code below affected two locations where Elixir uses Git:

* At build-time, Git is used to see if the build is inside a Git repo and if so, git info
  is added to the build information that Elixir returns with `System.build_info()`. The code
  uses PATH + `git` so the Git version from the inputs is used which is just fine.
* At run-time, Git is used by Mix if, and only if, there are any Git-based dependencies. This
  is not the normal case, and in that sense Git is an optional dependency of Elixir - most
  projects will work just fine without Git being present. Here, too, PATH + `git` is used
  so if there is a need on a user's system to use Git-based dependencies, the user can just
  add the package to the profile and things will just work.

I feel that leaving this alone is cleaner than trying to fix the dependency (if we want to
keep it, it needs to be in propagated-inputs in order to keep Git around; I stumbled upon
this building a project with Git dependencies after doing a `guix gc`. However, given that
using Git dependencies is the exception and I think that minimizing dependencies is nicer
I opted to just remove the substition completely).

---
 gnu/packages/elixir.scm | 5 -----
 1 file changed, 5 deletions(-)

--
2.34.0

Cees de Groot schreef op di 08-03-2022 om 20:52 [+0000]:
> I feel that leaving this alone is cleaner than trying to fix the dependency (if we want to
> keep it, it needs to be in propagated-inputs in order to keep Git around;

Instead of propagation, I recommend substitution, like done currently.
Propagation is not necessary to ‘keep git around’ (assuming that the
store reference is not obfuscated by gzipping or the like).

If it's used at build-time instead of run-time, then git should be
moved to native-inputs, for cross-compilation reasons.

Greetings,
Maxime

Cees de Groot schreef op di 08-03-2022 om 20:52 [+0000]:
> * At build-time, Git is used to see if the build is inside a Git repo and if so, git info

That cannot ever have worked in the first place, since Guix does not
save the '.git' directory when downloading elixir's source code.

Greetings,
Maxime.

Cees de Groot schreef op di 08-03-2022 om 20:52 [+0000]:
>   uses PATH + `git` so the Git version from the inputs is used which is just fine.

Due to cross-compilation reasons, this is not fine, it should use the
git from native-inputs instead.

Greetings,
Maxime.

Cees de Groot schreef op di 08-03-2022 om 20:52 [+0000]:
> * At run-time, Git is used by Mix if, and only if, there are any Git-based dependencies. This
>   is not the normal case, and in that sense Git is an optional dependency of Elixir - most
>   projects will work just fine without Git being present. Here, too, PATH + `git` is used
>   so if there is a need on a user's system to use Git-based dependencies, the user can just
>   add the package to the profile and things will just work.

If it's optional and not used most of the time, removing
it seems fine to me, though this seems useful information for the
description.  Also, it would be nice if 'elixir' were patched
to emit a nice message when 'git' is not found.  Suggestion:

  This project required 'git', but it is not in $PATH.  It can be
  installed into the user profile with `guix install git`.

You might want to contact whoever added the substitution originally,
to see what the original reasons were.

Greetings,
Maxime.

Cees de Groot schreef op wo 09-03-2022 om 02:39 [+0000]:
> > [...]
> > That cannot ever have worked in the first place, since Guix
> > does not save the '.git' directory when downloading elixir's source
> > code.

> That’s correct and fine. The info is informational at most  

How can being completely broken be correct and fine?

Greetings,
Maxime.

From the docs:

":revision - short Git revision hash. If Git was not available at building time, it is set to """ (https://hexdocs.pm/elixir/System.html#build_info/0)

So if git is not available - either because the executable is not there or because the build checkout is not from Git, the revision hash is set to the empty string, which is in accordance with the docs and what the Guix build has been doing all along (because, as you said, at build time, .git isn't there).

So for build time, cross-compiling or not, we don't need Git.

------- Original Message -------

On Wednesday, March 9th, 2022 at 02:31, Maxime Devos <maximedevos@telenet.be> wrote:

> Cees de Groot schreef op wo 09-03-2022 om 02:39 [+0000]:
>
> > > [...]
> > >
> > > That cannot ever have worked in the first place, since Guix
> > >
> > > does not save the '.git' directory when downloading elixir's source
> > >
> > > code.
>
> > That’s correct and fine. The info is informational at most
>
> How can being completely broken be correct and fine?
>
> Greetings,
>
> Maxime.

I think changing the behaviour of a programming language just to be helpful to developers is something that one may consider for upstream, but certainly not in the context of a packaging system like Guix - that just will breed confusion.

As it is, the error message thrown by Elixir when Git is not there is very easy to interpret for developers (and _only_ at development time can this happen!), no need to go overboard here. Unless, of course, you think it is useful to submit an upstream patch, feel free to do so.

------- Original Message -------

On Tuesday, March 8th, 2022 at 17:22, Maxime Devos <maximedevos@telenet.be> wrote:

> Cees de Groot schreef op di 08-03-2022 om 20:52 [+0000]:
>
> > * At run-time, Git is used by Mix if, and only if, there are any Git-based dependencies. This
> >
> >   is not the normal case, and in that sense Git is an optional dependency of Elixir - most
> >
> >   projects will work just fine without Git being present. Here, too, PATH + `git` is used
> >
> >   so if there is a need on a user's system to use Git-based dependencies, the user can just
> >
> >   add the package to the profile and things will just work.
>
> If it's optional and not used most of the time, removing
>
> it seems fine to me, though this seems useful information for the
>
> description. Also, it would be nice if 'elixir' were patched
>
> to emit a nice message when 'git' is not found. Suggestion:
>
> This project required 'git', but it is not in $PATH. It can be
>
> installed into the user profile with `guix install git`.
>
> You might want to contact whoever added the substitution originally,
>
> to see what the original reasons were.
>
> Greetings,
>
> Maxime.

> Propagation is not necessary to ‘keep git around’ (assuming that the

Well.... the whole reason that I stumbled upon this was that the compile-time substitution referred to a Git version that got GC'd. And I think that that is the different between regular inputs and propagated inputs, whether inputs become eligible for GC at run-time or not. If I'm misunderstanding something there, something else caused that version of Git to get GC'd and I'd be more than happy to be corrected in my assumptions.

On 9 March 2022 17:35:25 UTC, Cees de Groot <cg@evrl.com> wrote:
>> Propagation is not necessary to ‘keep git around’ (assuming that the
>
>Well.... the whole reason that I stumbled upon this was that the compile-time substitution referred to a Git version that got GC'd.

This simply Should Not happen.  Guix's GC model (inherited from Nix) is brutally simple: if the raw string /gnu/store/xxx occurs anywhere within a protected /gnu/store/yyy (e.g. in a binary), then xxx is also protected, recursively.

Assuming those assumptions hold here it's a 'mystery' why git got GC'd.  From experience and for our collective peace of mind I'd say it's more likely that they didn't hold somehow, than that there's a serious GC bug :-)

> And I think that that is the different between regular inputs and propagated inputs, whether inputs become eligible for GC at run-time or not.

So, no.  Completely unrelated.  The GC has no concept of time at all.  This isn't Gentoo.

Propagated inputs are 'oh, here's this extra thing I was unable to properly patch, please add it to the profile and treat it as a GC reference anyway'.

This is ugly and regular, non-propagated inputs are always preferred.  Some software just makes them too painful.
Cees,

Kind regards,

T G-R

Sent on the go.  Excuse or enjoy my brevity.

Cees de Groot schreef op wo 09-03-2022 om 17:31 [+0000]:
> I think changing the behaviour of a programming language just to be helpful to developers

Developers are users too and users can be developers.  Furthermore,
Elixir is an implementation of a programming language, so aren't all
users of Elixir automatically developers?

I'm not sure how exactly Elixir integrates with git but I assume the
integration can be very useful.

Also, this does not change the behaviour of the language, it only
adjusts a detail of the implementation: the error message for
indicating that git is missing is adjusted.

>  is something that one may consider for upstream, but certainly not in the context
>  of a packaging system like Guix - that just will breed confusion.

How could better error messages breed confusion?

Also, this seems a change that would absolutely _not_ be considered
upstream.  Why would upstream have distro-specific error messages?  If
the new error message is Guix-specific, wouldn't Guix be the logical
place to have it?

Greetings,
Maxime.

On Wednesday, March 9th, 2022 at 12:59, Tobias Geerinckx-Rice <me@tobias.gr> wrote:

> This simply Should Not happen. Guix's GC model (inherited from Nix) is brutally simple: if the raw string /gnu/store/xxx occurs anywhere within a protected /gnu/store/yyy (e.g. in a binary), then xxx is also protected, recursively.

So how does Guix figure out what to keep and what not? I mean, in this case the patched string will land in a compiled .BEAM file which very well may have the string obfuscated (I need to check how the VM stores strings in binaries).

Cees,

Yeah, this happens sometimes and will hide these references from what is a simple and therefore very fast string search.

It's why we install uncompressed .jar files and disable an obscure (and arguably unnoticable) GCC optimisation that hacks strings into incontiguous chunks.  If BEAM somehow obfuscates store references we'll have to find a similar work-around to stop it.  Let us know what you find.

Kind regards,

T G-R

Sent on the go.  Excuse or enjoy my brevity.

On Wednesday, March 9th, 2022 at 16:49, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
> If BEAM somehow obfuscates store references we'll have to find a similar work-around to stop it. Let us know what you find.
>
Yup. BEAM files have literals stored in a single Deflated blob (https://beam-wisdoms.clau.se/en/latest/indepth-beam-file.html). So the scanning won't work.

Which means that either my patches should be merged or Git should be propagated. I'm not a super big fan of propagating optional dependencies, which is why I submitted the patch like this.

Hi,

Cees de Groot <cg@evrl.com> skribis:

> On Wednesday, March 9th, 2022 at 16:49, Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>> If BEAM somehow obfuscates store references we'll have to find a similar work-around to stop it. Let us know what you find.
>>
> Yup. BEAM files have literals stored in a single Deflated blob (https://beam-wisdoms.clau.se/en/latest/indepth-beam-file.html). So the scanning won't work.

If we know what set of store references go in those blogs, we could
create a text file or something in #$output whose sole purpose is to
keep a reference to those store items.

For example, if we know those BEAM files contain a reference to Git, we
can add a phase like:

  (lambda* (#:key inputs outputs #:allow-other-keys)
    (symlink (search-input-file inputs "git")
             (string-append (assoc-ref output "out")
                            "/libexec/elixir/.git")))

That way the GC will keep Git around.

Thoughts?

Ludo’.

Ludovic Courtès schreef op vr 11-03-2022 om 22:51 [+0100]:
> If we know what set of store references go in those blogs, we could
> create a text file or something in #$output whose sole purpose is to
> keep a reference to those store items.
> 
> For example, if we know those BEAM files contain a reference to Git, we
> can add a phase like:
> 
>   (lambda* (#:key inputs outputs #:allow-other-keys)
>     (symlink (search-input-file inputs "git")

Doesn't that need to be "bin/git" instead of "git"?

>              (string-append (assoc-ref output "out")
>                             "/libexec/elixir/.git")))
> 
> That way the GC will keep Git around.
>
> Thoughts?

That is not sufficient if git acquires a graft.

Can elixir be patched to not compress the literals table (*)?
Alternatively, does deflate have some kind of "compression
level=0" setting -- perhaps we could do the same trick as
done for JARs?

(*) Preferably submitted upstream in some form, maybe with a
configuration flag?

Greetings,
Maxime.

Can’t we just run the patch? Then everything works just fine. I’m a full time Elixir developer and I can guarantee you that nothing will break :)

Sent from ProtonMail for iOS

On Fri, Mar 11, 2022 at 16:57, Maxime Devos <maximedevos@telenet.be> wrote:

> Ludovic Courtès schreef op vr 11-03-2022 om 22:51 [+0100]:
>> If we know what set of store references go in those blogs, we could
>> create a text file or something in #$output whose sole purpose is to
>> keep a reference to those store items.
>>
>> For example, if we know those BEAM files contain a reference to Git, we
>> can add a phase like:
>>
>> (lambda* (#:key inputs outputs #:allow-other-keys)
>> (symlink (search-input-file inputs "git")
>
> Doesn't that need to be "bin/git" instead of "git"?
>
>> (string-append (assoc-ref output "out")
>> "/libexec/elixir/.git")))
>>
>> That way the GC will keep Git around.
>>
>> Thoughts?
>
> That is not sufficient if git acquires a graft.
>
> Can elixir be patched to not compress the literals table (*)?
> Alternatively, does deflate have some kind of "compression
> level=0" setting -- perhaps we could do the same trick as
> done for JARs?
>
> (*) Preferably submitted upstream in some form, maybe with a
> configuration flag?
>
> Greetings,
> Maxime.

Cees de Groot schreef op vr 11-03-2022 om 22:01 [+0000]:
> Can’t we just run the patch? Then everything works just fine. I’m a
> full time Elixir developer and I can guarantee you that nothing will
> break :)

FWIW, I'm not convinced of the value of removing 'git', as I don't
expect a significant decrease in closure size and it makes Elixir a
little less usable (only a tiny bit because git can be installed
manually, but still a bit).

The only remaining reasons for removing it, appear to be avoiding
having to tackle the reference obfuscating issue -- sooner or later
(*), I expect there will eventually be an elixir-build-system and a few
elixir packages.  These elixir packages might need a few 'substitute*'-
style hardcoding.  So eventually, I expect the issue will reappear
again and frequently at that, so we might as well try to fix it now.

(*) Especially since there's an Elixir developer in Guix!

Greetings,
Maxime.

Well, feel free to do the patching work. Realize that you are going to do work for a) a very small group of developers (Elixir is a small language and Guix isn’t hardly a widely spread system to begin with), b) a subset of those that need git dependencies, and c) a subset of _those_ that need these dependencies on a project source tree that itself was not checked out with git (otherwise the git executable will already be in the environment and everything will work).

Please let me know when you found that unicorn developer :)

Sent from ProtonMail for iOS

On Fri, Mar 11, 2022 at 17:11, Maxime Devos <maximedevos@telenet.be> wrote:

> Cees de Groot schreef op vr 11-03-2022 om 22:01 [+0000]:
>> Can’t we just run the patch? Then everything works just fine. I’m a
>> full time Elixir developer and I can guarantee you that nothing will
>> break :)
>
> FWIW, I'm not convinced of the value of removing 'git', as I don't
> expect a significant decrease in closure size and it makes Elixir a
> little less usable (only a tiny bit because git can be installed
> manually, but still a bit).
>
> The only remaining reasons for removing it, appear to be avoiding
> having to tackle the reference obfuscating issue -- sooner or later
> (*), I expect there will eventually be an elixir-build-system and a few
> elixir packages. These elixir packages might need a few 'substitute*'-
> style hardcoding. So eventually, I expect the issue will reappear
> again and frequently at that, so we might as well try to fix it now.
>
> (*) Especially since there's an Elixir developer in Guix!
>
> Greetings,
> Maxime.

Cees de Groot schreef op vr 11-03-2022 om 22:36 [+0000]:
> Well, feel free to do the patching work. Realize that you are going
> to do work for a) a very small group of developers (Elixir is a small
> language and Guix isn’t hardly a widely spread system to begin with),
> b) a subset of those that need git dependencies, and c) a subset of
> _those_ that need these dependencies on a project source tree that
> itself was not checked out with git (otherwise the git executable
> will already be in the environment and everything will work). 

This is not only for the subset that uses git dependencies.  It is for
anyone writing and using Elixir packages, see my paragraph about
reference baking (in general, not git-specific):

> The only remaining reasons for removing it, appear to be avoiding
> having to tackle the reference obfuscating issue -- sooner or later
> (*), I expect there will eventually be an elixir-build-system and a
> few elixir packages. These elixir packages might need a few
> 'substitute*'- style hardcoding. So eventually, I expect the issue
> will reappear again and frequently at that, so we might as well try
> to fix it now.

More specifically, you could look at how, say, 'info-reader' uses
'substitute*', 'search-input-file', 'gunzip' and 'gzip'

Greetings,
Maxime.

If Guix ever gets elixir packages, that can be tackled.

I’m bowing out of this discussion. If my patch gets accepted then all good, if not, I’ll move it to my private channel and keep being happy there.

Sent from ProtonMail for iOS

On Fri, Mar 11, 2022 at 17:45, Maxime Devos <maximedevos@telenet.be> wrote:

> Cees de Groot schreef op vr 11-03-2022 om 22:36 [+0000]:
>> Well, feel free to do the patching work. Realize that you are going
>> to do work for a) a very small group of developers (Elixir is a small
>> language and Guix isn’t hardly a widely spread system to begin with),
>> b) a subset of those that need git dependencies, and c) a subset of
>> _those_ that need these dependencies on a project source tree that
>> itself was not checked out with git (otherwise the git executable
>> will already be in the environment and everything will work).
>
> This is not only for the subset that uses git dependencies. It is for
> anyone writing and using Elixir packages, see my paragraph about
> reference baking (in general, not git-specific):
>
>> The only remaining reasons for removing it, appear to be avoiding
>> having to tackle the reference obfuscating issue -- sooner or later
>> (*), I expect there will eventually be an elixir-build-system and a
>> few elixir packages. These elixir packages might need a few
>> 'substitute*'- style hardcoding. So eventually, I expect the issue
>> will reappear again and frequently at that, so we might as well try
>> to fix it now.
>
> More specifically, you could look at how, say, 'info-reader' uses
> 'substitute*', 'search-input-file', 'gunzip' and 'gzip'
>
> Greetings,
> Maxime.

Cees de Groot schreef op vr 11-03-2022 om 22:36 [+0000]:
> Well, feel free to do the patching work.

I'm just here reviewing things, looking for possible better
alternatives, etc.

>  Realize that you are going to do work for a) a very small group of
> developers (Elixir is a small language and Guix isn’t hardly a widely
> spread system to begin with), b) a subset of those that need git
> dependencies, and c) a subset of _those_ that need these dependencies
> on a project source tree that itself was not checked out with git
> (otherwise the git executable will already be in the environment and
> everything will work). 
> 
> Please let me know when you found that unicorn developer :)

a) You're an Elixir developer, using Guix, and doing packaging work for
   Elixir in Guix.  Also, how is the popularity of Guix relevant on
   guix-patches@, and how is the popularity of Elixir relevant when
   this whole patch is about improving the Elixir package?
b) does not seem relevant to me, see other e-mails (about removing a
   feature not worth the decrease in closure size IMO, and about
   non-git references)
c) It's a bit niche; it would be a very nice thing of Guix to support
   that niche.

   Also, being checked out with git does not imply that git is
   in the environment, it's possible to first set up a git repository
   and later enter a pure environment for development (guix shell --
   pure elixir elixir-foo elixir-bar).  I often do this (not with
   elixir, but with some other packages).

TBC, what do you mean with ‘unicorn developer’ here?

a) someone implementing this
b) someone in the subset

If (a), I was kind of hinting that you seem to be in a perfect position
to do so (you're an Elixir developer (*) in Guix and you seem to have
some knowledge of the serialisation formats).

(*) I'm assuming "Elixir developer" = "a developer of the Elixir
implementation" here.

> If Guix ever gets elixir packages, that can be tackled. 
> 
> I’m bowing out of this discussion. If my patch gets accepted then all
> good, if not, I’ll move it to my private channel and keep being happy
> there. 

Ok, but I'm a bit confused on why you don't seem interested in fixing
the reference issue and why you write ‘ever’ instead of ‘when’ because
you're both an Elixir and Guix developer.  (You don't have to answer,
not answering is fine.)

Greetings,
Maxime.

On Friday, March 11th, 2022 at 18:16, Maxime Devos <maximedevos@telenet.be> wrote:

> Ok, but I'm a bit confused on why you don't seem interested in fixing  the reference issue [...]

I am interested in fixing the bug in the current package. And, I'm quite honest there, I'm in itch-scratching mode so I'm not interested in fixing potential, future, maybe issues :). My patch works, and yes, I'm more than happy to improve and iterate on it _if_ - and to me, that's a very big if - more issues appear.

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op vr 11-03-2022 om 22:51 [+0100]:
>> If we know what set of store references go in those blogs, we could
>> create a text file or something in #$output whose sole purpose is to
>> keep a reference to those store items.
>> 
>> For example, if we know those BEAM files contain a reference to Git, we
>> can add a phase like:
>> 
>>   (lambda* (#:key inputs outputs #:allow-other-keys)
>>     (symlink (search-input-file inputs "git")
>
> Doesn't that need to be "bin/git" instead of "git"?
>
>>              (string-append (assoc-ref output "out")
>>                             "/libexec/elixir/.git")))
>> 
>> That way the GC will keep Git around.
>>
>> Thoughts?
>
> That is not sufficient if git acquires a graft.

Argh, good point.

Now, I agree with Cees that we need to be pragmatic here and opt for a
“good enough” solution.

So I went ahead and applied the initial patch that Cees posted.

However, that ‘replace-paths’ phase also touches references to /bin/sh;
are they also a problem after GC?

Thanks,
Ludo’.

These references only touch shell scripts so I think they should be just fine. I’ll give it a look when I’m back at work after the weekend to be sure though.

Sent from ProtonMail for iOS

On Sun, Mar 13, 2022 at 18:14, Ludovic Courtès <ludo@gnu.org> wrote:

> However, that ‘replace-paths’ phase also touches references to /bin/sh;
> are they also a problem after GC?
>
> Thanks,
> Ludo’.