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

[Leo Prikler <leo.prikler@student.tugraz.at>]

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