Hi Ludo,

thanks for your helpful comments!

>> +(define (channel-meta instance)
>> +  "Return an S-expression read from the channel INSTANCE's description file,
>> +or return #F if the channel instance does not include the file."
>> +  (let* ((source (channel-instance-checkout instance))
>> +         (meta-file (string-append source "/" %channel-meta-file)))
>> +    (and (file-exists? meta-file)
>> +         (call-with-input-file meta-file read))))
>
> As a general pattern, I’d suggest declaring <channel-metadata> record
> type along with a ‘read-channel-metadata’ procedure that takes care of
> “parsing” and metadata version handling.  That way parsing code is in
> just one place and the rest of the code can happily deal with
> well-formed records.

Yes, that’s a good idea.  I added a record <channel-metadata> and a
procedure “read-channel-metadata” that produces values of this type
given a channel instance (or #F).

>> +(define (channel-instance-dependencies instance)
>> +  "Return the list of channels that are declared as dependencies for the given
>> +channel INSTANCE."
>> +  (or (and=> (assoc-ref (channel-meta instance) 'dependencies)
>> +             (lambda (dependencies)
>> +               (map (lambda (item)
>> +                      (let ((get (lambda* (key #:optional default)
>> +                                   (or (and=> (assoc-ref item key) car) default))))
>> +                        (let ((name (get 'name))
>> +                              (url (get 'url))
>> +                              (branch (get 'branch "master"))
>> +                              (commit (get 'commit)))
>> +                          (and name url branch
>> +                               (channel
>> +                                (name name)
>> +                                (branch branch)
>> +                                (url url)
>> +                                (commit commit))))))
>> +                    dependencies)))
>> +      '()))
>
> I’d recommend ‘match’ for the outer sexp, and then something like the
> ‘alist-let*’ macro from (gnu services herd) in places where you’d like
> to leave field ordering unspecified.

I keep forgetting about alist-let* (from srfi-2, not herd), even though
it’s so useful!  “channel-instance-dependencies” now uses the
<channel-metadata> record via “read-channel-metadata” and uses match.

> Then I think it would make sense to add the ‘dependencies’ field to
> <channel-instance> directly (and keep <channel-metadata> internal.)
> Each element of the ‘dependencies’ field would be another
> <channel-instance>.
>
> Actually ‘dependencies’ could be a promise that reads channel meta-data
> and looks up the channel instances for the given dependencies.
> Something like that.

This sounds good, but I don’t know how to make it work well, because
there’s a circular relationship here if we want to keep the abstractions
pretty.  I can’t simply define the “dependencies” field of
<channel-instance> to have a default thunked procedure like this:

   (match (read-channel-metadata checkout)
     (#f '())
     (($ <channel-metadata> _ dependencies)
      dependencies))

Because record fields cannot access other record fields such as
“checkout”.  This makes the code look rather silly as we’re creating an
instance with an explicit dependencies value only to read it from that
same record in the next expression.

In light of these complications I’d prefer to have a procedure
“channel-instance-dependencies” that handles this for us, and do without
a “dependencies” field on the <channel-instance> record.

What do you think?

> Chris raises interesting issues.  I think it’s OK to first come up with
> an implementation that has some limitations but works with the simple
> use cases we have in mind.

I’ve fixed this according to what we’ve discussed: when more than one of
the user-provided or channel-required channels have the same name we
ignore the more recent specification unless it is more specific
(i.e. the new channel specification mentions a commit while the former
did not).

This is a little verbose because I replaced the simple “append-map” with
a more complex “fold” with a composite accumulator to avoid mutation.

Suggestions on how to simplify this are welcome!

--
Ricardo