Ludovic Courtès <[email protected]> writes:
> Hello!
>
> Tomas Volf <[email protected]> skribis:
>
>> after a pull, seeing the news, and subsequent rollback I went on
>> exploring how the new feature of the record system works. It seems to
>> work just for some fields (e.g., I can use it with inputs, but not with
>> version), which is quite confusing. Additionally, it does not seem to
>> allow cross-references (e.g., I cannot use inputs in propagated-inputs).
>
> It works for thunked fields only, just like self references such as
> ‘this-package’. (For reference, this was discussed at
> <https://codeberg.org/guix/guix/pulls/6955>.)
I see, I did not understood this from the news entry. That decreases
blast radius significantly, but it still is somewhat frustrating, since
thunk-ness of fields does not even seem to be documented? So one has to
always check the define-record-type to see what identifier are safe to
use when inheriting. That is not a great UX.
Hm, and when I stop/start inhering, the set of safe identifiers changes,
because this binding is injected only when inheriting, correct?
>
>> However the reason I am writing this email is this part from the news:
>>
>>> the newly introduced bindings could shadow same-named bindings
>>
>> which sounds quite annoying to deal with. Does this basically mean that
>> adding new field to any record in Guix can break arbitrary user code?
>
> The only risk is if you were doing:
>
> (define (foo inputs)
> (package
> (inherit xyz)
> ;; …
> (inputs (do-something inputs))))
>
> Previously the ‘inputs’ in the ‘inputs’ field body would refer to the
> outer ‘inputs’; now it refers to the inherited field value.
>
> It’s a very narrow case. In the entire code base, I found two
> occurrences of this.
I inspected my configurations and my channel, and I *think* I have no
occurrences, but hard to say, lot of define-record-type's to look
through. ¯\_(ツ)_/¯
It seems I mostly have "conflict" with `version', which is not a problem
(at least for now).
>
>> For now, I will spend the weekend checking that all my code does not
>> have any naming conflict, but I am not sure what to do next. Especially
>> given the fact that there is no warning when the shadowing occurs.
>>
>> I assume committers will ensure that no record will have fields that are
>> named the same way as any built-in procedure (and possibly some
>> hand-picked SRFIs and ICE-9s?), but in what matter shall I write my own
>> code? Is there some reserved prefix, I can be sure no record will ever
>> used for its fields? How should I future-proof my code?
>
> You’re raising good points. I think name clashes today are unlikely (in
> Guix itself we’d discover them not with warnings but with tests and CI
> for packages).
>
> Also, shadowing builtins is not a completely new thing. When you do:
>
> (package
> ;; …
> (version "1.2.3")
> (source (… (string-append … version ".tar.gz"))))
>
> … the ‘version’ in the body of ‘source’ is the one just above, not
> Guile’s builtin ‘version’ procedure. The extra shadowing (only when
> inheriting, only in thunked fields) doesn’t fundamentally change this.
True, and I am not fan even of this part of Guix's records. Though
until now, all shadowing was happening explicitly on the screen. Now,
whether something is shadowed changes whenever the thunk-ness changes,
without it being visible in my code. I fully admit I do not like that.
>
> Could you try your config/channels/code with this new feature and see if
> you stumble upon any issue?
I am building right now, and will see whether everything works.
>
> Thanks for your feedback!
I admit I do not like this feature, it feels way too magical and way too
unpredictable.
(let ((version "1"))
(package
(inherit foo)
(version version)))
What will package-version give me? "1" or (package-version foo)? There
is no way to know, based on the source code nor the documentation. I
have to dig into guix/packages.scm and consult the record definition.
And hope no one will change it in the future. ¯\_(ツ)_/¯
And I have to do this for every single record type I am using. Quick,
is guix-configuration-guix thunked or not? Well, I assume you do know
the answer, but I had to check. And I will have to check again after
next pull. :(
Tomas
PS:
Out of curiosity, was it ever considered to do away with the whole
(thunked) concept, and instead switch to explicit procedures taking the
record instance? I admit that
(package
...
(inputs (const (list ...))))
(package
...
(inputs (λ (pkg) (list ...))))
(package
...
(inputs (modify-inputs-lambda (delete "foo"))))
is slightly more verbose, but also significantly less magical.
--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
