| Message ID | e2bf165fc2905bcc8d33d23293eb3d31f3fbe4b8.1724911574.git.nigko.yerden@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#72867] gexp: Make 'local-file' follow symlinks. | expand |
pardon my ignorance, but can you give me a (plausible) example when someone wants to load some files relative to a source file, and also wants to be conscious of symlinks, and chose not to follow them? let alone making that the default anywhere around such operations? -- • attila lendvai • PGP: 963F 5D5F 45C7 DFCD 0A39 -- “An armed society is a polite society. Manners are good when one may have to back up his acts with his life.” — Robert Heinlein (1907–1988), 'Beyond This Horizon'
Hi Nigko, Nigko Yerden <nigko.yerden@gmail.com> skribis: > Fixes <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html> > > While the issue can be easily fixed (a one line change in 'absolute-dirname') > by changing 'current-source-directory' so that it always follows symlinks, > such a change may break someone else's code. Instead, this patch keeps the > original behavior of 'current-source-directory' macro and adds optional > 'follow-symlinks?' argument to it. > > This patch is the result of collective work of > Florian Pelz <pelzflorian@pelzflorian.de> and > Nigko Yerden <nigko.yerden@gmail.com> I haven’t read the thread above. Could you come up with a test case that shows the problem being fixed? (That is, the test should fail when run on current ‘master’.) That will allow us to “formalize” the issue and to make sure it doesn’t come back later. Thanks for your work, Ludo’.
Hello all. Thank you to Nigko for sending the patch. Nigko Yerden <nigko.yerden@gmail.com> writes: > This patch is the result of collective work of > Florian Pelz <pelzflorian@pelzflorian.de> and > Nigko Yerden <nigko.yerden@gmail.com> All real contribution to this patch is Nigko’s work. I contributed only the error location in a failed fix. Ludovic Courtès <ludo@gnu.org> writes: > I haven’t read the thread above. Could you come up with a test case > that shows the problem being fixed? (That is, the test should fail when > run on current ‘master’.) Nigko sums up the fixed issue in <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00071.html>: > pelzflorian (Florian Pelz) wrote: >> Nonsense; it must have worked; 7.7 Wrapping Up lists >> https://git.savannah.gnu.org/cgit/guile.git/tree/.guix/modules/guile-package.scm?id=cd57379b3df636198d8cd8e76c1bfbc523762e79 >> as proof. > […] > For me pulling from this channel with subsequent > > $ guix build guile@3.0.99-git > > throws an error ("No such file or directory" "GUILE-VERSION"). However, > > $ GUILE_LOAD_PATH= guix build guile@3.0.99-git > > , which emulates system without [1] in Guile load path, works like a charm. > Thus, this repository behaves exactly as does the main branch of [2]. > > Perhaps many systems (e.g. Guix on foreign distributions) indeed does not > have [1] in Guile load path, and thus recipe from the Cookbook works for them. > Regards, > Nigko > > [1] ~/.config/guix/current/share/guile/site/3.0/ > [2] https://gitlab.com/anigko/test-channel.git There are currently no tests for `current-source-directory'. To make a test case like in test/channels.scm, we would have to make a new guile process or build process, I presume? Regards, Florian
Hello Florian, > I contributed only the error location in a failed fix. Discussions and testings also should be counted. Without your suggestions I would hardly have made this patch. Thank you for all this. > There are currently no tests for `current-source-directory'. > To make a test case like in test/channels.scm, we would have to make > a new guile process or build process, I presume? I was thinking about making a test to 'local-file'. It is natural taking into account the problem this patch solves sits in 'local-file' bad behavior. But 'current-source-directory' is fine already. Regards, Nigko pelzflorian (Florian Pelz) wrote: > Hello all. Thank you to Nigko for sending the patch. > > Nigko Yerden <nigko.yerden@gmail.com> writes: >> This patch is the result of collective work of >> Florian Pelz <pelzflorian@pelzflorian.de> and >> Nigko Yerden <nigko.yerden@gmail.com> > > All real contribution to this patch is Nigko’s work. > I contributed only the error location in a failed fix. > > > Ludovic Courtès <ludo@gnu.org> writes: >> I haven’t read the thread above. Could you come up with a test case >> that shows the problem being fixed? (That is, the test should fail when >> run on current ‘master’.) > > Nigko sums up the fixed issue in > <https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00071.html>: >> pelzflorian (Florian Pelz) wrote: >>> Nonsense; it must have worked; 7.7 Wrapping Up lists >>> https://git.savannah.gnu.org/cgit/guile.git/tree/.guix/modules/guile-package.scm?id=cd57379b3df636198d8cd8e76c1bfbc523762e79 >>> as proof. >> […] >> For me pulling from this channel with subsequent >> >> $ guix build guile@3.0.99-git >> >> throws an error ("No such file or directory" "GUILE-VERSION"). However, >> >> $ GUILE_LOAD_PATH= guix build guile@3.0.99-git >> >> , which emulates system without [1] in Guile load path, works like a charm. >> Thus, this repository behaves exactly as does the main branch of [2]. >> >> Perhaps many systems (e.g. Guix on foreign distributions) indeed does not >> have [1] in Guile load path, and thus recipe from the Cookbook works for them. >> Regards, >> Nigko >> >> [1] ~/.config/guix/current/share/guile/site/3.0/ >> [2] https://gitlab.com/anigko/test-channel.git > > There are currently no tests for `current-source-directory'. > To make a test case like in test/channels.scm, we would have to make > a new guile process or build process, I presume? > > Regards, > Florian
No, I can't give you an example. The original 'current-source-directory' was designed not to follow symlinks. This wasn't my idea. By setting the default I just keep the original behavior. Regards, Nigko Attila Lendvai wrote: > pardon my ignorance, but can you give me a (plausible) example when someone > wants to load some files relative to a source file, and also wants to be > conscious of symlinks, and chose not to follow them? > > let alone making that the default anywhere around such operations? > > -- > • attila lendvai > • PGP: 963F 5D5F 45C7 DFCD 0A39 > -- > “An armed society is a polite society. Manners are good when one may have to > back up his acts with his life.” > — Robert Heinlein (1907–1988), 'Beyond This Horizon'
Nigko Yerden <nigko.yerden@gmail.com> writes: > Attila Lendvai wrote: >> pardon my ignorance, but can you give me a (plausible) example when >> someone wants to load some files relative to a source file, and also >> wants to be conscious of symlinks, and chose not to follow them? let >> alone making that the default anywhere around such operations? > No, I can't give you an example. The original 'current-source-directory' was > designed not to follow symlinks. This wasn't my idea. By setting the default > I just keep the original behavior. I guess not following symlinks was not design but an oversight. Profiles like .config/guix/current have lots of symlinks. Perhaps behavior might change when custom code is processing profiles. If we ignored possible custom code breakage, this patch could be simplified, but not to a one-liner, as it canonicalizes paths in both `current-source-directory' (when not in the load-path) and `absolute-dirname' (when in the load-path). Regards, Florian
Hi, pelzflorian (Florian Pelz) 写道： > If we ignored possible custom code breakage, this patch could be > simplified Please consider doing so, responsibly[0], if everyone agrees that the current default is suboptimal. Keeping ossified (and unintentional?) quirks around forever has a cost each time someone gets bitten by unintuitive behaviour. It gets less recognition than, but eventually outweighs, any immediate switching costs to out-of-tree users. (…/me quietly eyes substitute*…) Kind regards, T G-R [0]: With a news entry, for example.
diff --git a/guix/gexp.scm b/guix/gexp.scm index 74b4c49f90..5911ca4815 100644 --- a/guix/gexp.scm +++ b/guix/gexp.scm @@ -508,7 +508,7 @@ (define-syntax local-file (string? (syntax->datum #'file)) ;; FILE is a literal, so resolve it relative to the source directory. #'(%local-file file - (delay (absolute-file-name file (current-source-directory))) + (delay (absolute-file-name file (current-source-directory #t))) rest ...)) ((_ (assume-valid-file-name file) rest ...) ;; FILE is not a literal, so resolve it relative to the current diff --git a/guix/utils.scm b/guix/utils.scm index d8ce6ed886..b5fcf8cb28 100644 --- a/guix/utils.scm +++ b/guix/utils.scm @@ -1110,41 +1110,47 @@ (define (canonical-newline-port port) (define (%guix-source-root-directory) "Return the source root directory of the Guix found in %load-path." - (dirname (absolute-dirname "guix/packages.scm"))) + (dirname (absolute-dirname "guix/packages.scm" #f))) (define absolute-dirname ;; Memoize to avoid repeated 'stat' storms from 'search-path'. - (mlambda (file) + (mlambda (file follow-symlinks?) "Return the absolute name of the directory containing FILE, or #f upon -failure." +failure. Follow symlinks if FOLLOW-SYMLINKS? is true." (match (search-path %load-path file) (#f #f) ((? string? file) - ;; If there are relative names in %LOAD-PATH, FILE can be relative and - ;; needs to be canonicalized. - (if (string-prefix? "/" file) - (dirname file) - (canonicalize-path (dirname file))))))) + (if follow-symlinks? + (dirname (canonicalize-path file)) + ;; If there are relative names in %LOAD-PATH, FILE can be relative + ;; and needs to be canonicalized. + (if (string-prefix? "/" file) + (dirname file) + (canonicalize-path (dirname file)))))))) (define-syntax current-source-directory (lambda (s) "Return the absolute name of the current directory, or #f if it could not -be determined." +be determined. Do not follow symlinks if FOLLOW-SYMLINKS? is false (the default)." + (define (source-directory follow-symlinks?) + (match (assq 'filename (or (syntax-source s) '())) + (('filename . (? string? file-name)) + ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME + ;; can be relative. In that case, we try to find out at run time + ;; the absolute file name by looking at %LOAD-PATH; doing this at + ;; run time rather than expansion time is necessary to allow files + ;; to be moved on the file system. + (if (string-prefix? "/" file-name) + (dirname (if follow-symlinks? + (canonicalize-path file-name) + file-name)) + #`(absolute-dirname #,file-name #,follow-symlinks?))) + ((or ('filename . #f) #f) + ;; raising an error would upset Geiser users + #f))) (syntax-case s () - ((_) - (match (assq 'filename (or (syntax-source s) '())) - (('filename . (? string? file-name)) - ;; If %FILE-PORT-NAME-CANONICALIZATION is 'relative, then FILE-NAME - ;; can be relative. In that case, we try to find out at run time - ;; the absolute file name by looking at %LOAD-PATH; doing this at - ;; run time rather than expansion time is necessary to allow files - ;; to be moved on the file system. - (if (string-prefix? "/" file-name) - (dirname file-name) - #`(absolute-dirname #,file-name))) - ((or ('filename . #f) #f) - ;; raising an error would upset Geiser users - #f)))))) + ((_) (source-directory #f)) + ((_ follow-symlinks?) (source-directory #'follow-symlinks?))))) ;;;