[PATCH] Add '--symlink' to 'guix shell'

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • zimoun
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
Merged with
M
M
Maxim Cournoyer wrote on 10 Nov 2022 05:23
[PATCH v2 1/4] Makefile.am: Sort EXTRA_DIST entries.
(address . guix-patches@gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
20221110042351.829-1-maxim.cournoyer@gmail.com
* Makefile.am (EXTRA_DIST): Sort.
---
Makefile.am | 52 ++++++++++++++++++++++++++--------------------------
1 file changed, 26 insertions(+), 26 deletions(-)

Toggle diff (82 lines)
diff --git a/Makefile.am b/Makefile.am
index 47886721fa..c3af23b68e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -660,49 +660,49 @@ dist_fishcompletion_DATA = etc/completion/fish/guix.fish
nodist_selinux_policy_DATA = etc/guix-daemon.cil
EXTRA_DIST += \
- HACKING \
- ROADMAP \
- TODO \
- CODE-OF-CONDUCT \
.dir-locals.el \
.guix-authorizations \
.guix-channel \
- scripts/guix.in \
- etc/disarchive-manifest.scm \
- etc/guix-install.sh \
- etc/news.scm \
- etc/release-manifest.scm \
- etc/source-manifest.scm \
- etc/system-tests.scm \
- etc/time-travel-manifest.scm \
- etc/historical-authorizations \
+ CODE-OF-CONDUCT \
+ HACKING \
+ ROADMAP \
+ TODO \
+ bootstrap \
build-aux/build-self.scm \
- build-aux/compile-all.scm \
- build-aux/cuirass/hurd-manifest.scm \
- build-aux/check-final-inputs-self-contained.scm \
build-aux/check-channel-news.scm \
+ build-aux/check-final-inputs-self-contained.scm \
+ build-aux/compile-all.scm \
build-aux/compile-as-derivation.scm \
+ build-aux/config.rpath \
build-aux/convert-xref.scm \
+ build-aux/cuirass/hurd-manifest.scm \
build-aux/generate-authors.scm \
build-aux/test-driver.scm \
- build-aux/update-guix-package.scm \
build-aux/update-NEWS.scm \
- tests/test.drv \
+ build-aux/update-guix-package.scm \
+ doc/build.scm \
+ etc/disarchive-manifest.scm \
+ etc/guix-install.sh \
+ etc/historical-authorizations \
+ etc/news.scm \
+ etc/release-manifest.scm \
+ etc/source-manifest.scm \
+ etc/system-tests.scm \
+ etc/time-travel-manifest.scm \
+ scripts/guix.in \
tests/cve-sample.json \
- tests/keys/signing-key.pub \
- tests/keys/signing-key.sec \
tests/keys/civodul.pub \
- tests/keys/rsa.pub \
tests/keys/dsa.pub \
- tests/keys/ed25519.pub \
- tests/keys/ed25519.sec \
tests/keys/ed25519-2.pub \
tests/keys/ed25519-2.sec \
tests/keys/ed25519-3.pub \
tests/keys/ed25519-3.sec \
- build-aux/config.rpath \
- bootstrap \
- doc/build.scm \
+ tests/keys/ed25519.pub \
+ tests/keys/ed25519.sec \
+ tests/keys/rsa.pub \
+ tests/keys/signing-key.pub \
+ tests/keys/signing-key.sec \
+ tests/test.drv \
$(TESTS)
if !BUILD_DAEMON_OFFLOAD
--
2.37.3
M
M
Maxim Cournoyer wrote on 10 Nov 2022 14:42
control message for bug #59161
(address . control@debbugs.gnu.org)
87y1sjvsby.fsf@gmail.com
forcemerge 59161 59164
quit
M
M
Maxim Cournoyer wrote on 10 Nov 2022 14:42
control message for bug #59162
(address . control@debbugs.gnu.org)
87wn83vsbq.fsf@gmail.com
forcemerge 59162 59164
quit
M
M
Maxim Cournoyer wrote on 10 Nov 2022 14:42
control message for bug #59163
(address . control@debbugs.gnu.org)
87v8nnvsbk.fsf@gmail.com
forcemerge 59163 59164
quit
M
M
Maxim Cournoyer wrote on 10 Nov 2022 14:43
control message for bug #59164
(address . control@debbugs.gnu.org)
87tu37vsb2.fsf@gmail.com
forcemerge 59164 58812
quit
M
M
Maxim Cournoyer wrote on 15 Nov 2022 22:24
Re: bug#58812: [PATCH v3 1/4] shell: Detect --symlink spec problems early.
(address . 58812-done@debbugs.gnu.org)(address . 59164-done@debbugs.gnu.org)
87y1sblxls.fsf@gmail.com
Hi,

[...]

Toggle quote (6 lines)
> Makefile.am: Sort EXTRA_DIST entries.
> tests: Add a tests/utils.sh support file.
> install: Validate symlink target in evaluate-populate-directive.
> guix: shell: Add '--symlink' option.
> shell: Detect --symlink spec problems early.

I've now pushed this series as 8f9588185d, with a news entry added as
47f319f21f.

Closing!

--
Thanks,
Maxim
Closed
L
L
Ludovic Courtès wrote on 17 Nov 2022 18:31
control message for bug #59164
(address . control@debbugs.gnu.org)
87zgcpfpxk.fsf@gnu.org
retitle 59164 [PATCH] Add '--symlink' to 'guix shell'
quit
L
L
Ludovic Courtès wrote on 17 Nov 2022 18:37
Coding style: similarly-named variables
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87sfihfpng.fsf_-_@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (28 lines)
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> * gnu/build/install.scm (evaluate-populate-directive): By default, error when
>>> the target of a symlink doesn't exist. Always ensure TARGET ends with "/".
>>> (populate-root-file-system): Call evaluate-populate-directive with
>>> #:error-on-dangling-symlink #t and add comment.
>>
>> [...]
>>
>>> + (define target* (if (string-suffix? "/" target)
>>> + target
>>> + (string-append target "/")))
>>
>> Maybe make it:
>>
>> (let ((target (if …)))
>> …)
>>
>> so there’s only one ‘target’ in scope (and no ‘target*’); otherwise it’s
>> easy to forget the ‘*’ and refer to wrong one.
>
> It's a pattern I've used at other places; I find it more hygienic to not
> shadow existing variables; it signal to the reader "be careful, this is
> not the same as the argument-bound one, though they are closely
> related".

I don’t buy it. :-) The reader might be careful yet end up using the
“wrong” variable. As long as the “wrong” variable has no use, I think
it’s best to shadow it so that mistakes cannot happen.

Of course the details vary depending on context, but I think we should
not start introducing this pattern in different places. Perhaps
something to discuss and codify under “Formatting Code”?

Ludo’.
M
M
Maxim Cournoyer wrote on 17 Nov 2022 21:34
(name . Ludovic Courtès)(address . ludo@gnu.org)
87v8ndtj58.fsf@gmail.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (36 lines)
> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>>
>>>> * gnu/build/install.scm (evaluate-populate-directive): By default, error when
>>>> the target of a symlink doesn't exist. Always ensure TARGET ends with "/".
>>>> (populate-root-file-system): Call evaluate-populate-directive with
>>>> #:error-on-dangling-symlink #t and add comment.
>>>
>>> [...]
>>>
>>>> + (define target* (if (string-suffix? "/" target)
>>>> + target
>>>> + (string-append target "/")))
>>>
>>> Maybe make it:
>>>
>>> (let ((target (if …)))
>>> …)
>>>
>>> so there’s only one ‘target’ in scope (and no ‘target*’); otherwise it’s
>>> easy to forget the ‘*’ and refer to wrong one.
>>
>> It's a pattern I've used at other places; I find it more hygienic to not
>> shadow existing variables; it signal to the reader "be careful, this is
>> not the same as the argument-bound one, though they are closely
>> related".
>
> I don’t buy it. :-) The reader might be careful yet end up using the
> “wrong” variable. As long as the “wrong” variable has no use, I think
> it’s best to shadow it so that mistakes cannot happen.

I'm surprised you're not buying it, given we're writing Scheme in a more
functional style, and mutating same-named variables clearly goes against
that style :-).

Toggle quote (4 lines)
> Of course the details vary depending on context, but I think we should
> not start introducing this pattern in different places. Perhaps
> something to discuss and codify under “Formatting Code”?

That's more of a coding style guidelines than "formatting" code (when I
read "formatting", I think of a mechanical process like 'guix style' or
'rust-fmt' can do), but yes, that could be nice to have. Better yet,
something basic to share across the whole Guile/Scheme community and
include in the Guile user manual, like Python has PEP 8 they can refer
to, to save every Guile/Scheme project from having to reinvent the
wheel.

--
Thanks,
Maxim
Z
Z
zimoun wrote on 17 Nov 2022 19:44
Re: [bug#59164] Coding style: similarly-named variables
86zgcpju9p.fsf@gmail.com
Hi,

On Thu, 17 Nov 2022 at 18:37, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (13 lines)
>> It's a pattern I've used at other places; I find it more hygienic to not
>> shadow existing variables; it signal to the reader "be careful, this is
>> not the same as the argument-bound one, though they are closely
>> related".
>
> I don’t buy it. :-) The reader might be careful yet end up using the
> “wrong” variable. As long as the “wrong” variable has no use, I think
> it’s best to shadow it so that mistakes cannot happen.
>
> Of course the details vary depending on context, but I think we should
> not start introducing this pattern in different places. Perhaps
> something to discuss and codify under “Formatting Code”?

I agree with Ludo. For another instance than target*, the previous was,

Toggle snippet (7 lines)
((new '-> old)
[...]
- (symlink old (string-append target new)))
[...]
- (delete-file (string-append target new))

then replaced by,

Toggle snippet (9 lines)
((new '-> old)
[...]
+ (let ((new* (string-append target* new)))
[...]
+ (error (format #f "symlink `~a' points to nonexistent \
+file `~a'" new* old)))))
+ (symlink old new*))

Well, it seems a Star War. ;-) As Ludo, I am not convinced that it is
less error-prone, maybe the contrary.


Cheers,
simon
M
M
Maxim Cournoyer wrote on 18 Nov 2022 18:02
(name . zimoun)(address . zimon.toutoune@gmail.com)
87y1s82o23.fsf@gmail.com
Hi,

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (36 lines)
> Hi,
>
> On Thu, 17 Nov 2022 at 18:37, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> It's a pattern I've used at other places; I find it more hygienic to not
>>> shadow existing variables; it signal to the reader "be careful, this is
>>> not the same as the argument-bound one, though they are closely
>>> related".
>>
>> I don’t buy it. :-) The reader might be careful yet end up using the
>> “wrong” variable. As long as the “wrong” variable has no use, I think
>> it’s best to shadow it so that mistakes cannot happen.
>>
>> Of course the details vary depending on context, but I think we should
>> not start introducing this pattern in different places. Perhaps
>> something to discuss and codify under “Formatting Code”?
>
> I agree with Ludo. For another instance than target*, the previous was,
>
> ((new '-> old)
> [...]
> - (symlink old (string-append target new)))
> [...]
> - (delete-file (string-append target new))
>
>
> then replaced by,
>
> ((new '-> old)
> [...]
> + (let ((new* (string-append target* new)))
> [...]
> + (error (format #f "symlink `~a' points to nonexistent \
> +file `~a'" new* old)))))
> + (symlink old new*))

The intent was to keep away from the following imperative style, which
hurts both readability and debuggability in my opinion:

Toggle snippet (6 lines)
(let* ((my-target "something")
(my-target (mutate-once my-target))
(my-target (mutate-twice my-target)))
(do-something-with my-target))

Perhaps the problem at hand would benefit being broken down in smaller
chunks, to avoid having a page-full of code sharing the same scope.

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 20 Nov 2022 11:46
Re: Coding style: similarly-named variables
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87pmdh7vkn.fsf@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

[...]

Toggle quote (13 lines)
>>> It's a pattern I've used at other places; I find it more hygienic to not
>>> shadow existing variables; it signal to the reader "be careful, this is
>>> not the same as the argument-bound one, though they are closely
>>> related".
>>
>> I don’t buy it. :-) The reader might be careful yet end up using the
>> “wrong” variable. As long as the “wrong” variable has no use, I think
>> it’s best to shadow it so that mistakes cannot happen.
>
> I'm surprised you're not buying it, given we're writing Scheme in a more
> functional style, and mutating same-named variables clearly goes against
> that style :-).

There’s no mutation here, only lexical scoping. Anyway, I find it clear
that the risk of typing ‘x’ instead of ‘x*’, especially in relatively
long functions, justifies shadowing in situations like this one. WDYT?

Toggle quote (6 lines)
>> Of course the details vary depending on context, but I think we should
>> not start introducing this pattern in different places. Perhaps
>> something to discuss and codify under “Formatting Code”?
>
> That's more of a coding style guidelines than "formatting" code

Sorry I meant “Coding Style”, which is the section that documents the
project’s conventions.

Toggle quote (7 lines)
> (when I read "formatting", I think of a mechanical process like 'guix
> style' or 'rust-fmt' can do), but yes, that could be nice to have.
> Better yet, something basic to share across the whole Guile/Scheme
> community and include in the Guile user manual, like Python has PEP 8
> they can refer to, to save every Guile/Scheme project from having to
> reinvent the wheel.

I won’t do it, but sure, why not! My immediate concern is to make sure
we have a shared understanding, within Guix, of some of the conventions
we follow. It’s a minor issue, but minor issues are what our day-to-day
work is made of. :-)

Thanks,
Ludo’.
Z
Z
zimoun wrote on 21 Nov 2022 16:02
Re: [bug#58812] [bug#59164] Coding style: similarly-named variables
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87zgckwdtw.fsf@gmail.com
Hi Maxim,

On Fri, 18 Nov 2022 at 12:02, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

Toggle quote (10 lines)
> The intent was to keep away from the following imperative style, which
> hurts both readability and debuggability in my opinion:
>
> --8<---------------cut here---------------start------------->8---
> (let* ((my-target "something")
> (my-target (mutate-once my-target))
> (my-target (mutate-twice my-target)))
> (do-something-with my-target))
> --8<---------------cut here---------------end--------------->8---

Well, ’mutate-*’ is not really mutating. Maybe I miss something and
from my understanding, this ’let*’reads,

Toggle snippet (6 lines)
(let ((my-target "something"))
(let ((my-target (mutate-once my-target)))
(let ((my-target (mutate-twice my-target)))
(do-something-with my-target))))

and not,

Toggle snippet (7 lines)
(begin
(define my-target "something")
(set! my-target (mutate-once my-target))
(set! my-target (mutate-twice my-target))
(do-something-with my-target))

Well, the former is ’lexical-scope’d so the 3 ’my-target’ are not truly
an imperative style, I guess.

Back to the pattern, you are suggesting to write,

Toggle snippet (6 lines)
(let* ((my-target "something")
(my-target* (mutate-once my-target))
(my-target** (mutate-twice my-target*)))
(do-something-with my-target**))

well, I am not convinced it helps for readibility. And I think, the
pattern is manually doing what ’let*’ is already doing for you.

Cheers,
simon
Z
Z
zimoun wrote on 21 Nov 2022 16:52
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87mt8kwbhv.fsf@gmail.com
On Mon, 21 Nov 2022 at 16:02, zimoun <zimon.toutoune@gmail.com> wrote:

Toggle quote (10 lines)
> Well, ’mutate-*’ is not really mutating. Maybe I miss something and
> from my understanding, this ’let*’reads,
>
> --8<---------------cut here---------------start------------->8---
> (let ((my-target "something"))
> (let ((my-target (mutate-once my-target)))
> (let ((my-target (mutate-twice my-target)))
> (do-something-with my-target))))
> --8<---------------cut here---------------end--------------->8---

Well, it compiles to something similar…

Toggle quote (3 lines)
> And I think, the
> pattern is manually doing what ’let*’ is already doing for you.

…for instance, it reads,

Toggle snippet (16 lines)
scheme@(guix-user)> (macroexpand
'(let* ((my-target "something")
(my-target (mutate-once my-target))
(my-target (mutate-twice my-target)))
(do-something-with my-target)))

$1= #<tree-il
(let (my-target) (my-target-11e760207b4c89cb-114)
((const "something"))
(let (my-target) (my-target-11e760207b4c89cb-116)
((call (toplevel mutate-once) (lexical my-target my-target-11e760207b4c89cb-114)))
(let (my-target) (my-target-11e760207b4c89cb-118)
((call (toplevel mutate-twice) (lexical my-target my-target-11e760207b4c89cb-116)))
(call (toplevel do-something-with) (lexical my-target my-target-11e760207b4c89cb-118)))))>

Cheers,
simon
M
M
Maxim Cournoyer wrote on 21 Nov 2022 21:55
(name . zimoun)(address . zimon.toutoune@gmail.com)
87sficqb71.fsf@gmail.com
Hi Simon,

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (31 lines)
> Hi Maxim,
>
> On Fri, 18 Nov 2022 at 12:02, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> The intent was to keep away from the following imperative style, which
>> hurts both readability and debuggability in my opinion:
>>
>> --8<---------------cut here---------------start------------->8---
>> (let* ((my-target "something")
>> (my-target (mutate-once my-target))
>> (my-target (mutate-twice my-target)))
>> (do-something-with my-target))
>> --8<---------------cut here---------------end--------------->8---
>
> Well, ’mutate-*’ is not really mutating. Maybe I miss something and
> from my understanding, this ’let*’reads,
>
> (let ((my-target "something"))
> (let ((my-target (mutate-once my-target)))
> (let ((my-target (mutate-twice my-target)))
> (do-something-with my-target))))
>
>
> and not,
>
> (begin
> (define my-target "something")
> (set! my-target (mutate-once my-target))
> (set! my-target (mutate-twice my-target))
> (do-something-with my-target))

Right. I used "mutated" where I should have used "shadowed by lexical
scoping". The outcome for me is the same; the original value of an
argument (target) in the code gets shadowed, thus is theory it becomes
more difficult to inspect its original value, should we have a debugger
that is able to stop at the place to inspect to print ',locals'.

In practice since using breakpoints/a debugger to debug Guile code
rarely works as intended (in my experience hacking on Guix!), we
typically sprinkle the source with 'pk', and that point becomes moot.

Toggle quote (13 lines)
> Well, the former is ’lexical-scope’d so the 3 ’my-target’ are not truly
> an imperative style, I guess.
>
> Back to the pattern, you are suggesting to write,
>
> (let* ((my-target "something")
> (my-target* (mutate-once my-target))
> (my-target** (mutate-twice my-target*)))
> (do-something-with my-target**))

> well, I am not convinced it helps for readibility. And I think, the
> pattern is manually doing what ’let*’ is already doing for you.

The value it provides is that it becomes easy to inspect each
intermediary result in a debugger.

I think we're done expressing the arguments to have on both sides, which
aren't too strong either ways :-). I'm happy to restrain myself using
such a pattern and keep moving forward.

--
Thanks,
Maxim
Z
Z
zimoun wrote on 22 Nov 2022 15:35
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
86fsebdpl9.fsf@gmail.com
Hi Maxim,

On Mon, 21 Nov 2022 at 15:55, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

Toggle quote (4 lines)
> In practice since using breakpoints/a debugger to debug Guile code
> rarely works as intended (in my experience hacking on Guix!), we
> typically sprinkle the source with 'pk', and that point becomes moot.

I totally agree! Preparing some materials for introducing Guile to
GuixHPC folk, I am trying to collect some tips and, if I am honest, the
debugging experience with Guile is really poor; compared to others (as
Python). For example, DrRacket provides an easy and nice user
experience [1] – where it is easy to compare each intermediary result in
the debugger. For what it is worth, I have not been able to have some
similar inspections as in [1]. Maybe, I am missing something…

Well, IMHO, we are somehow suffering from some Guile limitations and
improvements in this area are an hard task.

Cheers,
simon

Short video demoing (link will be dead after 2022-12-07)
L
L
Ludovic Courtès wrote on 26 Nov 2022 15:47
(name . zimoun)(address . zimon.toutoune@gmail.com)
87pmd993i4.fsf@gnu.org
Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

Toggle quote (8 lines)
> I totally agree! Preparing some materials for introducing Guile to
> GuixHPC folk, I am trying to collect some tips and, if I am honest, the
> debugging experience with Guile is really poor; compared to others (as
> Python). For example, DrRacket provides an easy and nice user
> experience [1] – where it is easy to compare each intermediary result in
> the debugger. For what it is worth, I have not been able to have some
> similar inspections as in [1]. Maybe, I am missing something…

Looking at the video you posted, I better understand what debugging
features we’re talking about. DrRacket is the gold standard; here it
does something similar to what we have with in Elisp with EDebug, which
is certainly useful.

It may be more of a limitation of Geiser than of Guile. I find it more
useful in “typical” imperative ELisp code than in functional Scheme
code, but it’d be nice to have either way!

Ludo’.
?