[bug#49828] [PATCH 06/20] guix: Add ContentDB importer. (XXX Yes send now)

[Leo Prikler]

Hi,

Am Montag, den 09.08.2021, 22:00 +0200 schrieb Maxime Devos:
> [...]
> The URL would look like:
> /packages/<author>/<name>/releases/<release>/download/.
> Here, <release> is a the ‘release id’, which is an integer (e.g.
> 4209).  It is _not_ the version number, but it is monotonically
> increasing.
> 
> There are some problems however:
> 
>   * Old archives are sometimes deleted.
> 
>     TenPlus1/ethereal was added to ContentDB on 2018-02-23, but it 
>     only has a single release on ContentDB, from 2021-07-28
>     [...]
>     Likewise for TenPlus1/bakedclay, TenPlus1/wine, TenPlus1/bees.
> 
>     Most other mods still have the old archives though (e.g., 
>     Jeija/mesecons, sfan5/worldedit, PilzAdam/nether).  The mods by 
>     TenPlus1 seems to be an exception.
> 
>   * The version number is not included in the download URL, the 
>     release id is.
>     So IIUC, update-package-source in (guix upstream) would still 
>     need to be adjusted somewhat to support Minetest packages.
> 
> +(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).
> > 
> 
> I don't see how a 'minetest-uri' would simplify the definition of
> 'make-package-sexp'.  Using 'minetest-uri' would avoid the need
> to specify the commit, but 'minetest-uri' needs a release id anyway,
> so no simplification there.
> 
> I guess it would avoid the 'download-git-repository' procedure
> and 'vcs-file?', but see two points above.  Also, 'latest-repository-
> commit' returns a store path, which does not include the '.git'
> directory, so 'vcs-file?' isn't necessary, so I removed 'vcs-file?'.
In other words, taking minetest-uri as is currently doesn't seem too
good of an idea.  Does the plain git/vcs updater work for those
packages then?  I assume at least some of the packages should be tagged
properly in git, are they not?

> [...]
> I like the name 'package-full-name'.  <package> and <package-keys> 
> are rather different structures and used in different contexts
> though, so I kept package-full-name and package-keys-full-name
> separate.
> 
> FWIW, I added a procedure
> 
> (define (%construct-full-name author name)
>   (string-append author "/" name))
> 
> used by 'package-full-name' and 'package-keys-full-name'.
Sounds good to me.

> > > +(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
> 
> ContentDB has a policy of requiring mod names to be unique in order
> to be a ‘approved’, so I don't think name conflict will be a problem
> in practice.  If full names were generated, keep in mind that
> dependencies would need to use the full names a well.  I couldn't
> find any mods with name conflicts.  I would just emit a warning for
> now.
Fair enough, if that requirement is actually enforced by ContentDB,
that's good.  It does make the AUTHOR/NAME URI syntax look a bit weird
tho.

> contentdb->package-name was always used together with 'author/name-
> >name', so I modified contentdb->package-name to call author/name-
> >name as you seem to suggest.  It maked the code a little simpler.
Well, that's one way of resolving this issue, another would have been
to move the comment to where it makes sense.

All in all, I think I'm rather content with this patch now, but I have
a final nitpick w.r.t the updater.  "upstream-name" is a weird name for
a property that will supposedly be used by only one updater (and even
if different updaters were to use it, would not each one have slightly
different, but perhaps sometimes overlapping expectations for that
field?)  I think a better solution would be to set home-page to the
ContentDB page, assuming that is acceptable.  If not, perhaps having a
"refresh-url" that is well defined over all refreshers and supersedes
home-page if specified might be a more appropriate solution.  

WDYT?