diff mbox series

[bug#49828,06/20] guix: Add ContentDB importer.

Message ID e534dbff9977126289db0c0eb6cb1281613a877b.camel@telenet.be
State Accepted
Headers show
Series None | expand

Commit Message

M Aug. 7, 2021, 6:31 p.m. UTC 
Leo Prikler schreef op do 05-08-2021 om 18:41 [+0200]:
> Hi,
> 
> Am Montag, den 02.08.2021, 17:50 +0200 schrieb Maxime Devos:
> > * guix/import/contentdb.scm: New file.
> > * guix/scripts/import/contentdb.scm: New file.
> > * tests/contentdb.scm: New file.
> > * Makefile.am (MODULES, SCM_TESTS): Register them.
> > * po/guix/POTFILES.in: Likewise.
> > * doc/guix.texi (Invoking guix import): Document it.
> > [...]
> > diff --git a/doc/guix.texi b/doc/guix.texi
> > index 43c248234d..d06c9b73c5 100644
> > --- a/doc/guix.texi
> > +++ b/doc/guix.texi
> > @@ -11313,6 +11313,31 @@ and generate package expressions for all
> > those packages that are not yet
> >  in Guix.
> >  @end table
> >  
> > +@item contentdb
> > +@cindex ContentDB
> > +Import metadata from @uref{https://content.minetest.net, ContentDB}.
> > +Information is taken from the JSON-formatted metadata provided
> > through
> > +@uref{https://content.minetest.net/help/api/, ContentDB's API} and
> > +includes most relevant information, including dependencies.  There
> > are
> > +some caveats, however.  The license information on ContentDB does
> > not
> > +distinguish between GPLvN-only and GPLvN-or-later.

ContentDB uses SPDX license identifiers now, and distinguishes between
GPL-N-only and GPL-N-or-later, so I adjusted the documentation appropriately.

> >   The commit id is
> > +sometimes missing.  The descriptions are in the Markdown format, but
> > +Guix uses Texinfo instead.  Texture packs and subgames are
> > unsupported.
> What is the "commit id"?  Is it the hash?  A tag?  Anything that
> resolves to a commit?

It's the SHA-1 of the Git commit.  I changes this to ‘the commit's SHA-1’.

> Also, since ContentDB sounds fairly generic (a database of content?),
> perhaps we ought to call this the "minetest" importer instead?

Technically, minetest has another mod repository as well:
<https://bower.minetest.org/>.  It's unmoderated though, and
<https://content.minetest.net> has some moderation and seems more
‘official’ (it's integrated in Minetest itself).  I replaced
(guix import contentdb) with (guix import minetest), likewise
for (guix script import minetest) and tests/minetest.scm.

> > +;; Minetest package.
> > +;;
> > +;; API endpoint: /packages/AUTHOR/NAME/
> > +(define-json-mapping <package> make-package package?
> > +  json->package
> > +  (author            package-author) ; string
> > +  (creation-date     package-creation-date ; string
> > +                     "created_at")
> > +  (downloads         package-downloads) ; integer
> > +  (forums            package-forums "forums" natural-or-false) ;
> > natural | #f
> This comment and some others like it seem to simply be repeating
> already present information.  Is there a use for them?  Should we
> instead provide a third argument on every field to verify/enforce the
> type?

I first added the ‘; natural-or-false’.  I only added the procedure
"natural-false" later.  Indeed, ‘; natural-or-false’ is redundant.
I removed the redundant ones in the revised patch.

I don't think there is need to verify types for each field.
Most aren't used by Guix.  If a type check would fail, that would
presumably mean the type check guix is incorrect (or not up-to-date).
Except for perhaps a backtrace, ill-typed fields are harmless.

> > +(define (contentdb-fetch author name)
> > +  "Return a <package> record for package NAME by AUTHOR, or #f on
> > failure."
> > +  (and=> (json-fetch
> > +          (string-append (%contentdb-api) "packages/" author "/"
> > name "/"))
> > +         json->package))
> Is there a reason for author and name to be separate keys?  For me it
> makes more sense to take AUTHOR/NAME as a singular search string from
> users and then perform queries based on that.

Not really actually, AUTHOR tends to go togehter with NAME except for
some exceptions.  I modified the code such that AUTHOR/NAME is a single
string.  It simplified code somewhat.

>   If ContentDB allows
> searching, we might also resolve NAME to a singular package where
> possible and otherwise error out, telling the user to choose one.

ContentDB allows searching.  I wrote some a procedure 'elaborate-contentdb-name'
used by (guix scripts import contentdb) that resolves "mesecons" to "Jeija/mesecons",
using the search API and added some tests.  If there are multiple candidates,
the one with the highest ‘score’ is choosen (alternatively, --sort=downloads can
be used instead).

> > +(define (important-dependencies dependencies author name)
> > +  (define dependency-list
> > +    (assoc-ref dependencies (string-append author "/" name)))
> > +  (filter-map
> > +   (lambda (dependency)
> > +     (and (not (dependency-optional? dependency))
> > +          ;; "default" must be provided by the 'subgame' in use
> > +          ;; and does not refer to a specific minetest mod.
> > +          ;; "doors", "bucket" ... are provided by the default
> > minetest
> > +          ;; subgame.
> > +          (not (member (dependency-name dependency)
> > +                       '("default" "doors" "beds" "bucket" "doors"
> > "farming"
> > +                         "flowers" "stairs" "xpanes")))

I tested this some more, and it appears that some mods depend on "dyes",
which is part of the default Minetest game, so I added all the mods
provided by the default (sub?)game.  The list began looking a little
long, so I replaced it with a hash table.

> > +          ;; Dependencies often have only one implementation.
> > +          (let* ((/name (string-append "/" (dependency-name
> > dependency)))
> > +                 (likewise-named-implementations
> > +                  (filter (cut string-suffix? /name <>)
> > +                          (dependency-packages dependency)))
> > +                 (implementation
> > +                  (and (not (null? likewise-named-implementations))
> > +                       (first likewise-named-implementations))))
> > +            (and implementation
> > +                 (apply cons (string-split implementation #\/))))))
> > +   dependency-list))
> What exactly does the likewise-named-implementations bit do here?

The list returned by 'dependency-packages' not only contains the mod
we need, but possibly also various ‘subgames’ that include that mod.
Filtering on '/name' filters out these subgames we don't need.

Also, theoretically another mod could implement the same interface.
The filtering would filter out the alternative implementations.

Anyway, I changes the implementation a bit.  It now explicitely
filters out ‘subgames’ and ‘texture packs’ using the ‘package-mod?’
procedure.  The resulting list tends to consist of only a single
element.  If it consists of multiple, the one with the highest score
(or the one with the highest download count, depending on --sort)
will be choosen (and a warning is printed).

> > +;; A list of license names is available at
> > +;; <https://content.minetest.net/api/licenses/>;;.
> > +(define (string->license str)
> > +  "Convert the string STR into a license object." [...]
> The link mentions, that ContentDB now supports all SPDX identifiers. 
> Do we have a SPDX->Guix converter lying around in some other importer
> that we could use as default case here (especially w.r.t. "or later")

There's a a converter in (guix import utils): spdx-string->license.
The old license identifiers appear to be removed, now only SPDX information
is available.  I modified the code to use spdx->string-license and removed
string->license.

It turns out it does not recognise GPL-N-only and GPL-N-or-later,
so I added a patch ‘import/utils: Recognise GPL-3.0-or-later and friends.’.

I tried implementing "guix refresh -t minetest ...".  It seems to work, but
requires some changes to (guix upstream) that needs some more work, so I left it
out of the revised patch set.  The refresher needs to know the author name
(or perform extra HTTP requests), so I added 'upstream-name' the package properties.

The revised patch series is attached.  It can also be found at
<https://notabug.org/maximed/guix-gnunet/src/minetest-2>.  It includes
the latest MINETEST_MOD_PATH patch.  I'll make the patch to export more things in
(guix build utils) later (for core-updates).

reetings,
Maxime.

Comments

Leo Prikler Aug. 7, 2021, 7:47 p.m. UTC | #1 
Hi,

Am Samstag, den 07.08.2021, 20:31 +0200 schrieb Maxime Devos:
> [...]
> ContentDB uses SPDX license identifiers now, and distinguishes
> between GPL-N-only and GPL-N-or-later, so I adjusted the
> documentation appropriately.
Nice.

> > > The commit id is
> > > +sometimes missing.  The descriptions are in the Markdown format,
> > > but
> > > +Guix uses Texinfo instead.  Texture packs and subgames are
> > > unsupported.
> > What is the "commit id"?  Is it the hash?  A tag?  Anything that
> > resolves to a commit?
> 
> It's the SHA-1 of the Git commit.  I changes this to ‘the commit's
> SHA-1’.
We usually call it the hash around here :)

> > Also, since ContentDB sounds fairly generic (a database of
> > content?),
> > perhaps we ought to call this the "minetest" importer instead?
> 
> Technically, minetest has another mod repository as well:
> <https://bower.minetest.org/>;.  It's unmoderated though, and
> <https://content.minetest.net> has some moderation and seems more
> ‘official’ (it's integrated in Minetest itself).  I replaced
> (guix import contentdb) with (guix import minetest), likewise
> for (guix script import minetest) and tests/minetest.scm.
If we get more of these we could label them like elpa repos, but I
don't think we'll get many third party repos with ContentDB existing.

> > > +;; Minetest package.
> > > +;;
> > > +;; API endpoint: /packages/AUTHOR/NAME/
> > > +(define-json-mapping <package> make-package package?
> > > +  json->package
> > > +  (author            package-author) ; string
> > > +  (creation-date     package-creation-date ; string
> > > +                     "created_at")
> > > +  (downloads         package-downloads) ; integer
> > > +  (forums            package-forums "forums" natural-or-false) ;
> > > natural | #f
> > This comment and some others like it seem to simply be repeating
> > already present information.  Is there a use for them?  Should we
> > instead provide a third argument on every field to verify/enforce
> > the
> > type?
> 
> I first added the ‘; natural-or-false’.  I only added the procedure
> "natural-false" later.  Indeed, ‘; natural-or-false’ is redundant.
> I removed the redundant ones in the revised patch.
> 
> I don't think there is need to verify types for each field.
> Most aren't used by Guix.  If a type check would fail, that would
> presumably mean the type check guix is incorrect (or not up-to-date).
> Except for perhaps a backtrace, ill-typed fields are harmless.
Fair enough, gratuitous redundancy is the bigger issue here.

> [...]
> ContentDB allows searching.  I wrote some a procedure 'elaborate-
> contentdb-name' used by (guix scripts import contentdb) that resolves
> "mesecons" to "Jeija/mesecons", using the search API and added some
> tests.  If there are multiple candidates, the one with the highest
> ‘score’ is choosen (alternatively, --sort=downloads can be used
> instead).
Sounds good to me.

> > > +(define (important-dependencies dependencies author name)
> > > +  (define dependency-list
> > > +    (assoc-ref dependencies (string-append author "/" name)))
> > > +  (filter-map
> > > +   (lambda (dependency)
> > > +     (and (not (dependency-optional? dependency))
> > > +          ;; "default" must be provided by the 'subgame' in use
> > > +          ;; and does not refer to a specific minetest mod.
> > > +          ;; "doors", "bucket" ... are provided by the default
> > > minetest
> > > +          ;; subgame.
> > > +          (not (member (dependency-name dependency)
> > > +                       '("default" "doors" "beds" "bucket"
> > > "doors"
> > > "farming"
> > > +                         "flowers" "stairs" "xpanes")))
> 
> I tested this some more, and it appears that some mods depend on
> "dyes",
> which is part of the default Minetest game, so I added all the mods
> provided by the default (sub?)game.  The list began looking a little
> long, so I replaced it with a hash table.
> 
> > > +          ;; Dependencies often have only one implementation.
> > > +          (let* ((/name (string-append "/" (dependency-name
> > > dependency)))
> > > +                 (likewise-named-implementations
> > > +                  (filter (cut string-suffix? /name <>)
> > > +                          (dependency-packages dependency)))
> > > +                 (implementation
> > > +                  (and (not (null? likewise-named-
> > > implementations))
> > > +                       (first likewise-named-implementations))))
> > > +            (and implementation
> > > +                 (apply cons (string-split implementation
> > > #\/))))))
> > > +   dependency-list))
> > What exactly does the likewise-named-implementations bit do here?
> 
> The list returned by 'dependency-packages' not only contains the mod
> we need, but possibly also various ‘subgames’ that include that mod.
> Filtering on '/name' filters out these subgames we don't need.
> 
> Also, theoretically another mod could implement the same interface.
> The filtering would filter out the alternative implementations.
> 
> Anyway, I changes the implementation a bit.  It now explicitely
> filters out ‘subgames’ and ‘texture packs’ using the ‘package-mod?’
> procedure.  The resulting list tends to consist of only a single
> element.  If it consists of multiple, the one with the highest score
> (or the one with the highest download count, depending on --sort)
> will be choosen (and a warning is printed).
Sounds good.

> > > +;; A list of license names is available at
> > > +;; <https://content.minetest.net/api/licenses/>;;;.
> > > +(define (string->license str)
> > > +  "Convert the string STR into a license object." [...]
> > The link mentions, that ContentDB now supports all SPDX
> > identifiers. 
> > Do we have a SPDX->Guix converter lying around in some other
> > importer
> > that we could use as default case here (especially w.r.t. "or
> > later")
> 
> There's a a converter in (guix import utils): spdx-string->license.
> The old license identifiers appear to be removed, now only SPDX
> information is available.  I modified the code to use spdx->string-
> license and removed string->license.
Nice.

> It turns out it does not recognise GPL-N-only and GPL-N-or-later,
> so I added a patch ‘import/utils: Recognise GPL-3.0-or-later and
> friends.’.
Said patch LGTM.

> I tried implementing "guix refresh -t minetest ...".  It seems to
> work, but requires some changes to (guix upstream) that needs some
> more work, so I left it out of the revised patch set.  The refresher
> needs to know the author name (or perform extra HTTP requests), so I
> added 'upstream-name' the package properties.
Could we somehow define a (minetest-uri) procedure that can be supplied
to (guix download)?  Somehow Minetest must get the package to
installations across operating systems, so surely there's some download
link somewhere.

> The revised patch series is attached.  It can also be found at
> <https://notabug.org/maximed/guix-gnunet/src/minetest-2>;.  It
> includes the latest MINETEST_MOD_PATH patch.  I'll make the patch to
> export more things in (guix build utils) later (for core-updates).
For the rest of this, I'll only look over 06/20 v2.  I'll assume you
did nothing naughty to 01..04.

> +;; A structure returned by the /api/packages/?fmt=keys endpoint
> +(define-json-mapping <package/keys> make-package/keys package/keys?
> +  json->package/keys
> +  (author package/keys-author) ; string
> +  (name package/keys-name)     ; string
> +  (type package/keys-type))    ; string
Not sure about this slash, as it typically specifies extension of some
sort.  Perhaps just naming this package-keys would be better?

> +(define (package-author/name package)
> +  "Given a <package> object, return the corresponding AUTHOR/NAME
> string."
> +  (string-append (package-author package) "/" (package-name
> package)))
> +
> +(define (package/keys-author/name package)
> +  "Given a <package/keys> object, return the corresponding
> AUTHOR/NAME string."
> +  (string-append (package/keys-author package)
> +                 "/" (package/keys-name package)))
I think it's probably be better to merge this into a single procedure
called "package-full-name", "package-unique-name" or "package-id"
(whichever you prefer naming-wise), which handles both cases.

> +(define (contentdb->package-name name)
> +  "Given the NAME of a package on ContentDB, return a Guix-compliant 
> name for the
> +package."
> +  ;; The author is not included, as the names of popular mods
> +  ;; tend to be unique.
> +  (string-append "minetest-" (snake-case name)))
I'd at least add an option to generate full names instead, for cases in
which as before we warn about non-uniqueness.  Though actually, this
comment is a little misleading as the actual stripping happens...
> +     (name ,(contentdb->package-name (author/name->name
> author/name)))
here.

> +(define* (make-package-sexp #:key
> +                            (guix-name "minetest-foo")
> +                            (home-page "https://example.org/foo")
> +                            (repo "https://example.org/foo.git")
> +                            (synopsis "synopsis")
> +                            (guix-description "description")
> +                            (guix-license
> +                             '(list license:cc-by-sa4.0
> license:lgpl3+))
> +                            (inputs '())
> +                            (upstream-name "Author/foo")
> +                            #:allow-other-keys)
> + [...]
As noted above, this procedure would be somewhat simplified if we could
define a (mintest-uri).

Regards
diff mbox series

Patch

From 33e15111c00d9988d8123a674930a27b61e891b3 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sun, 1 Aug 2021 20:03:07 +0200
Subject: [PATCH 20/20] gnu: Add minetest-homedecor-modpack.

* gnu/packages/minetest.scm
  (minetest-homedecor-modpack): New variable.
---
 gnu/packages/minetest.scm | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/gnu/packages/minetest.scm b/gnu/packages/minetest.scm
index f49e0080f2..ca9d0ac720 100644
--- a/gnu/packages/minetest.scm
+++ b/gnu/packages/minetest.scm
@@ -107,6 +107,37 @@  special items, intending to make an interesting adventure.")
       (license (list license:cc0 license:expat))
       (properties `((upstream-name . "TenPlus1/ethereal"))))))
 
+(define-public minetest-homedecor-modpack
+  (package
+    (name "minetest-homedecor-modpack")
+    ;; Upstream doesn't tag releases, so use the release title from
+    ;; ContentDB as version.
+    (version "2021-03-27-1")
+    (source
+     (origin
+       (method git-fetch)
+       (uri (git-reference
+             (url "https://gitlab.com/VanessaE/homedecor_modpack")
+             (commit "9ffe2b7d691133e1a067546574fbe7364fd02f32")))
+       (sha256
+        (base32 "1lfajqvc2adf9hqskghky4arccqzpjw4i9a01hv4qcckvivm04ag"))
+       (file-name (git-file-name name version))))
+    (build-system minetest-mod-build-system)
+    (propagated-inputs
+     `(("minetest-basic-materials" ,minetest-basic-materials)
+       ("minetest-unifieddyes" ,minetest-unifieddyes)))
+    (home-page (minetest-topic 2041))
+    (synopsis "Home decor mod for Minetest")
+    (description
+     ;; TRANSLATORS: ‘homedecor’ is the name is the name of a Minetest mod
+     ;; and should not be translated.
+     "The homedecor Minetest mod provides a large seleection of items that
+might be found inside and around homes, such as sofas, chairs, tables, fences
+and a variety of other stuff.")
+    (license
+     (list license:cc-by-sa4.0 license:lgpl3))
+    (properties `((upstream-name . "VanessaE/homedecor_modpack")))))
+
 (define-public minetest-mesecons
   ;; The release on ContentDB does not have its own version number.
   (let ((commit "db5879706d04d3480bc4863ce0c03fa73e5f10c7")
-- 
2.32.0