GNU bug report logs - #66436
[PATCH] doc: Add some guidelines for reviewing.

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS).
Date: Sat, 14 Oct 2023 19:12:42 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * guix/git-download.scm (<git-reference>) [lfs?]: New field.
> (git-fetch/in-band): New #:git-lfs argument.
> <inputs>: Remove labels.  Conditionally add git-lfs.
> <build>: Read "git lfs?" environment
> variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
> Use INPUTS directly; update comment.
> <gexp->derivation>: Add "git lfs?" to #:env-vars.
> (git-fetch/built-in): Add "lfs?" to #:env-vars.
> * guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
> (git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
> * guix/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?'
> environment variable.
> * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?'
> field.
>  (Requirements): Mention the optional 'git-lfs' dependency.
> * configure.ac: Add a check for the 'git-lfs' command.
> * guix/config.scm.in (%git-lfs): New variable.
> * guix/self.scm (%packages): Add git-lfs.
> (compiled-guix): Add git-lfs to guix-config.
> (make-config.scm): New #:git-lfs argument.

I wonder whether this is a desirable feature, in the sense that the
store is not designed to hold large amounts of data: just like Git has
Git-LFS and Git-Annex, we’d need Guix-Annex (in fact, the GWL is kinda
doing that already!).

Furthermore…

> +dnl Git Large File Storage is an optional dependency for the
> +dnl "builtin:git-download" derivation builder.
> +AC_PATH_PROG([GIT_LFS], [git-lfs])
> +if test "x$GIT_LFS" = "x"; then
> +  AC_MSG_WARN([Git Large File Storage (git-lfs) is missing;
> +  The builtin:git-download derivation builder of the Guix daemon will
> +  not be able to use it.])

… I don’t think we want to spend more words on the effect of increasing
the closure size and getting locked with Git (libgit2 doesn’t implement
LFS I suppose.)  To me this part is a showstopper; we should just make
it clear that “builtin:git-download” does not implement LFS.

Also, it is crucial for the “builtin:git-download” semantics to be the
same across all installations and to be very stable.

> +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs
>                            (package-name "GNU Guix")
>                            (package-version "0")
>                            (channel-metadata #f)
> @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git
>                                 %store-database-directory
>                                 %config-directory
>                                 %git
> +                               %git-lfs

This I’d like to avoid, too (for the size issue).

Overall, I feel like LFS support, if needed, should be “on the side”,
with a custom ‘git-fetch/lfs’ or something along these lines (just like
we have ‘url-fetch/tarbomb’).

We might still need to change (guix build git) to implement it, but I
would make sure that “builtin:git-download” remains unaffected and never
fetches LFS stuff, even if ‘git-lfs’ happens to be on $PATH or
something.

WDYT?

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.