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: > ;. It's unmoderated though, and > 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 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 > > > +;; ;;;. > > > +(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 > ;. 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 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 object, return the corresponding AUTHOR/NAME > string." > + (string-append (package-author package) "/" (package-name > package))) > + > +(define (package/keys-author/name package) > + "Given a 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