| Message ID | 20221027035100.28852-3-maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add --symlink option to 'guix shell'. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git-branch | success | View Git branch |
| cbaines/applying patch | success | |
| cbaines/issue | success | View issue |
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. > + (let ((new* (string-append target* new))) Likewise. > + (when error-on-dangling-symlink? > + ;; When the symbolic link points to a relative path, > + ;; checking if its target exists must be done relative to > + ;; the link location. > + (with-directory-excursion (if (string-prefix? "/" old) > + (getcwd) > + (dirname new*)) ;relative > + (unless (file-exists? old) > + (error (format #f "symlink `~a' points to nonexistent \ > +file `~a'" new* old))))) > + (symlink old new*)) I would avoid the directory excursion when unnecessary: (unless (if (string-prefix? "/" old) (file-exists? old) (with-directory-excursion (dirname new) (file-exists? old))) …) (We could use ‘lstat’ instead of ‘file-exists?’ if we want to allow symlinks to dangling symlinks…) Ludo’.
Hi again, 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". >> + (when error-on-dangling-symlink? >> + ;; When the symbolic link points to a relative path, >> + ;; checking if its target exists must be done relative to >> + ;; the link location. >> + (with-directory-excursion (if (string-prefix? "/" old) >> + (getcwd) >> + (dirname new*)) ;relative >> + (unless (file-exists? old) >> + (error (format #f "symlink `~a' points to nonexistent \ >> +file `~a'" new* old))))) >> + (symlink old new*)) > > I would avoid the directory excursion when unnecessary: > > (unless (if (string-prefix? "/" old) > (file-exists? old) > (with-directory-excursion (dirname new) > (file-exists? old))) > …) Done: --8<---------------cut here---------------start------------->8--- modified gnu/build/install.scm @@ -99,14 +99,14 @@ (define target* (if (string-suffix? "/" target) (lambda () (when error-on-dangling-symlink? ;; When the symbolic link points to a relative path, - ;; checking if its target exists must be done relative to - ;; the link location. - (with-directory-excursion (if (string-prefix? "/" old) - (getcwd) - (dirname new*)) ;relative - (unless (file-exists? old) - (error (format #f "symlink `~a' points to nonexistent \ -file `~a'" new* old))))) + ;; checking if its target exists must be done relatively + ;; to the link location. + (unless (if (string-prefix? "/" old) + (file-exists? old) + (with-directory-excursion (dirname new*) + (file-exists? old))) + (error (format #f "symlink `~a' points to nonexistent \ +file `~a'" new* old)))) (symlink old new*)) --8<---------------cut here---------------end--------------->8--- > (We could use ‘lstat’ instead of ‘file-exists?’ if we want to allow > symlinks to dangling symlinks…) It seems better to leave it as-is; the odd use case of symlinking to a dangling symlink can be accomplished via "#:error-on-dangling-symlink #f" :-).
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. 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’.
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, --8<---------------cut here---------------start------------->8--- ((new '-> old) [...] - (symlink old (string-append target new))) [...] - (delete-file (string-append target new)) --8<---------------cut here---------------end--------------->8--- then replaced by, --8<---------------cut here---------------start------------->8--- ((new '-> old) [...] + (let ((new* (string-append target* new))) [...] + (error (format #f "symlink `~a' points to nonexistent \ +file `~a'" new* old))))) + (symlink old new*)) --8<---------------cut here---------------end--------------->8--- Well, it seems a Star War. ;-) As Ludo, I am not convinced that it is less error-prone, maybe the contrary. Cheers, simon
Hi, Ludovic Courtès <ludo@gnu.org> writes: > 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 :-). > 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.
Hi, zimoun <zimon.toutoune@gmail.com> writes: > 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: --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--- 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.
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: [...] >>> 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? >> 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. > (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’.
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, --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--- and not, --8<---------------cut here---------------start------------->8--- (begin (define my-target "something") (set! my-target (mutate-once my-target)) (set! my-target (mutate-twice my-target)) (do-something-with my-target)) --8<---------------cut here---------------end--------------->8--- 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, --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, 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
On Mon, 21 Nov 2022 at 16:02, zimoun <zimon.toutoune@gmail.com> wrote: > 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… > And I think, the > pattern is manually doing what ’let*’ is already doing for you. …for instance, it reads, --8<---------------cut here---------------start------------->8--- 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)))))> --8<---------------cut here---------------end--------------->8--- Cheers, simon
Hi Simon, zimoun <zimon.toutoune@gmail.com> writes: > 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. > 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.
Hi Maxim, On Mon, 21 Nov 2022 at 15:55, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > 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) 1: https://filesender.renater.fr/?s=download&token=92d4312a-91b4-402e-898a-40ce01a5c3ed
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > 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’.
diff --git a/gnu/build/install.scm b/gnu/build/install.scm index f5c8407b89..15cc29b2c8 100644 --- a/gnu/build/install.scm +++ b/gnu/build/install.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2016 Chris Marusich <cmmarusich@gmail.com> +;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -56,19 +57,24 @@ (define (install-boot-config bootcfg bootcfg-location mount-point) (define* (evaluate-populate-directive directive target #:key (default-gid 0) - (default-uid 0)) + (default-uid 0) + (error-on-dangling-symlink? #t)) "Evaluate DIRECTIVE, an sexp describing a file or directory to create under directory TARGET. DEFAULT-UID and DEFAULT-GID are the default UID and GID in the context of the caller. If the directive matches those defaults then, -'chown' won't be run." +'chown' won't be run. When ERROR-ON-DANGLING-SYMLINK? is true, abort with an +error when a dangling symlink would be created." + (define target* (if (string-suffix? "/" target) + target + (string-append target "/"))) (let loop ((directive directive)) (catch 'system-error (lambda () (match directive (('directory name) - (mkdir-p (string-append target name))) + (mkdir-p (string-append target* name))) (('directory name uid gid) - (let ((dir (string-append target name))) + (let ((dir (string-append target* name))) (mkdir-p dir) ;; If called from a context without "root" permissions, "chown" ;; to root will fail. In that case, do not try to run "chown" @@ -78,27 +84,38 @@ (define* (evaluate-populate-directive directive target (chown dir uid gid)))) (('directory name uid gid mode) (loop `(directory ,name ,uid ,gid)) - (chmod (string-append target name) mode)) + (chmod (string-append target* name) mode)) (('file name) - (call-with-output-file (string-append target name) + (call-with-output-file (string-append target* name) (const #t))) (('file name (? string? content)) - (call-with-output-file (string-append target name) + (call-with-output-file (string-append target* name) (lambda (port) (display content port)))) ((new '-> old) - (let try () - (catch 'system-error - (lambda () - (symlink old (string-append target new))) - (lambda args - ;; When doing 'guix system init' on the current '/', some - ;; symlinks may already exists. Override them. - (if (= EEXIST (system-error-errno args)) - (begin - (delete-file (string-append target new)) - (try)) - (apply throw args)))))))) + (let ((new* (string-append target* new))) + (let try () + (catch 'system-error + (lambda () + (when error-on-dangling-symlink? + ;; When the symbolic link points to a relative path, + ;; checking if its target exists must be done relative to + ;; the link location. + (with-directory-excursion (if (string-prefix? "/" old) + (getcwd) + (dirname new*)) ;relative + (unless (file-exists? old) + (error (format #f "symlink `~a' points to nonexistent \ +file `~a'" new* old))))) + (symlink old new*)) + (lambda args + ;; When doing 'guix system init' on the current '/', some + ;; symlinks may already exists. Override them. + (if (= EEXIST (system-error-errno args)) + (begin + (delete-file new*) + (try)) + (apply throw args))))))))) (lambda args ;; Usually we can only get here when installing to an existing root, ;; as with 'guix system init foo.scm /'. @@ -142,7 +159,10 @@ (define* (populate-root-file-system system target includes /etc, /var, /run, /bin/sh, etc., and all the symlinks to SYSTEM. EXTRAS is a list of directives appended to the built-in directives to populate TARGET." - (for-each (cut evaluate-populate-directive <> target) + ;; It's expected that some symbolic link targets do not exist yet, so do not + ;; error on dangling links. + (for-each (cut evaluate-populate-directive <> target + #:error-on-dangling-symlink? #f) (append (directives (%store-directory)) extras)) ;; Add system generation 1.