[PATCH] gexp: Make 'local-file' follow symlinks.

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>

argument.

(%guix-source-root-directory): Pass #f to 'absolute-dirname'

'follow-symlinks?' argument.

(current-source-directory): Add 'follow-symlinks?' optional argument.

'follow-symlinks?' argument.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

guix/gexp.scm | 2 +-

guix/utils.scm | 52 ++++++++++++++++++++++++++++----------------------

2 files changed, 30 insertions(+), 24 deletions(-)

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:

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:

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:

Nigko sums up the fixed issue in

https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00071.html:

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,

Discussions and testings also should be counted. Without your

suggestions I would hardly have made this patch.

Thank you for all this.

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:

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:

Nigko Yerden <nigko.yerden@gmail.com> writes:

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) ???

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.

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>

argument.

(%guix-source-root-directory): Pass #f to 'absolute-dirname'

'follow-symlinks?' argument.

(current-source-directory): Add 'follow-symlinks?' optional argument.

'follow-symlinks?' argument.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

Hello Ludo, Florian,

Add test to 'local-file'.

Regards,

Nigko

guix/gexp.scm | 2 +-

guix/utils.scm | 52 ++++++++++++++++++++++++++++----------------------

tests/gexp.scm | 23 ++++++++++++++++++++++

3 files changed, 53 insertions(+), 24 deletions(-)

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>

argument.

(%guix-source-root-directory): Pass #f to 'absolute-dirname'

'follow-symlinks?' argument.

(current-source-directory): Add 'follow-symlinks?' optional argument.

'follow-symlinks?' argument.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

Forgot to unwrap #'follow-symlinks? syntax object with 'syntax->datum'

when calling 'source-directory' inside 'current-source-directory'.

guix/gexp.scm | 2 +-

guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------

tests/gexp.scm | 23 ++++++++++++++++++++++

3 files changed, 54 insertions(+), 24 deletions(-)

Hello Nigko.

Nigko Yerden <nigko.yerden@gmail.com> writes:

Thanks for the credit, but it would be unusual to mention me in the

commit message, where discussion does not count.

Please do not put me in the commit message; I made no code contribution.

I also would favor to simplify `current-source-directory' and not add an

optional follow-symlinks? argument. I believe processing profiles is

the only reasonable case that unconditionally following symlinks would

break, and people do not do profile processing in outside code.

This one is a good test; but it tests only half, namely the

rare-in-practice case of `local-file' when loading a Scheme file. Here,

`current-source-directory' evaluate file-name to

"/tmp/guix-directory.VxrxZT/dir/link-to-code.scm", which has a slash as

prefix, so absolute-dirname is not called.

The original issue is that the package in a channel according to

cookbook’s “The Repository as a Channel” cannot be built when the

load-path is set up in the usual way. There, absolute-dirname gets

called. I think we would need a (very similar) test that covers this.

Instead of primitive-load, we would need to invoke Guile on a file in a

channel or in the GUILE_LOAD_PATH, or set %load-path. I may be wrong

here and do not know how, but we definitely should cover when

`file-name' in is not prefixed with a slash.

Regards,

Florian

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.

;;; Copyright © 2024 Nigko Yerden <nigko.yerden@gmail.com>

argument.

(%guix-source-root-directory): Pass #f to 'absolute-dirname'

'follow-symlinks?' argument.

(current-source-directory): Add 'follow-symlinks?' optional argument.

'follow-symlinks?' argument.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

Hello Florian,

pelzflorian (Florian Pelz) wrote:

Thanks for noticing this. Indeed 'absolute-dirname' was not called.

I have fixed this by turning 'code.scm' into a module 'test-local-file.scm'

and loading it twice: first using 'use-module' and then via 'load'

(for some unclear reason 'primitive-load' causes an error here, so

I replaced it with 'load').

OK, I removed your name from the commit message.

Regards,

Nigko

guix/gexp.scm | 2 +-

guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------

tests/gexp.scm | 33 +++++++++++++++++++++++++++++++

3 files changed, 64 insertions(+), 24 deletions(-)

Hello Florian,

pelzflorian (Florian Pelz) wrote:

Why do you think that making 'current-source-directory' to always follow

symlinks would not break Guix's own code as well?

What are these people whose code would be broken supposed to do? I think

they would need to write their own 'current-source-directory' from scratch.

Why not help them by providing 'follow-symlinks?' argument?

Instead of 'current-source-directory' simplification we can also consider

changing the default for 'follow-symlinks?' to #t.

Regards,

Nigko

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.

;;; Copyright © 2024 Nigko Yerden <nigko.yerden@gmail.com>

argument.

(%guix-source-root-directory): Pass #f to 'absolute-dirname'

'follow-symlinks?' argument.

(current-source-directory): Add 'follow-symlinks?' optional argument.

'follow-symlinks?' argument.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

Using of 'eval' in test from v4 is wrong. It does not play any role there.

Most importantly it does not prevent spoiling of '%load-path' for the

rest of 'tests/gexp.scm' module. Here is the better version of the test

that uses 'dynamic-wind'.

guix/gexp.scm | 2 +-

guix/utils.scm | 53 ++++++++++++++++++++++++++++----------------------

tests/gexp.scm | 33 +++++++++++++++++++++++++++++++

3 files changed, 64 insertions(+), 24 deletions(-)

Hello Nigko, Tobias, Ludo, Attila (putting them in Cc again).

Nigko Yerden <nigko.yerden@gmail.com> writes:

The test is beautiful. It makes clear why each

canonicalize-path is needed. I had not understood the issue entirely

before I read it.

When we follow symlinks, both calling the real "../test-local-file.scm"

and the symlink to it behaves the same.

When not following symlinks, we get different results depending on what

we run. I see no reason to ever want that except displaying info or

debugging.

And except that not following symlinks is faster (fewer stat syscalls).

But Ludovic wrapped absolute-dirname in a memoizing mlambda in commit

87b711d200ad13eaef284bdd1ab77f85618b0498, which reduces the difference.

Regarding the code, if we kept the old code when `follow-symlinks?' is

false (we should not, but if we did), it remains surprising that we do

follow some symlinks.

In the new use-modules part of the test, `file-name'

in `current-source-directory' is "./test-local-file.scm" code, which

means if we look at what happens if we not follow-symlinks, we took the

latter (canonicalize-path (dirname file)) path in

which does not follow the symlink in the basename and fails.

But it would follow symlinks in the directory part.

Regards,

Florian

Hello Nigko.

They would not have to write code like `current-source-directory'.

Also note that such affected users had broken code when running the

real file.

`local-file' with absolute paths always did `(canonicalize-path (dirname`

and does not change.

`dirname' being called in a special case of `current-source-directory',

`canonicalize-path' as part of `absolute-file-name'.

Could we finish this bug report by applying your nice test code, but

changing only `absolute-dirname' to do (canonicalize-path (dirname))

in all cases?

Then either add no news item, or write the above?

Regards,

Florian

Fix https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html

via making 'current-source-directory' always follow symlinks.

them follow symlinks.

Change-Id: Ieb30101275deb56b7436df444f9bc21d240fba59

---

Hello all,

This version of patch advocated by Florian changes 'current-source-directory'

to always follow symlinks.

Regards,

Nigko

guix/utils.scm | 8 ++------

tests/gexp.scm | 33 +++++++++++++++++++++++++++++++++

2 files changed, 35 insertions(+), 6 deletions(-)

Hi Nigko,

Nigko Yerden <nigko.yerden@gmail.com> skribis:

[...]

Am I right that we cannot keep the ‘if’ here, as it would perform

“lexical” dot-dot resolution instead of Unix resolution (accounting for

symlinks), right?

Note that ‘current-source-directory’ is a macro; using

‘canonicalize-path’ here could lead to an exception being thrown at

macro-expansion time, if ‘file-name’ doesn’t exist. This normally

doesn’t happen but maybe we should handle this gracefully?

The downside of these two changes is that this leads to potentially many

‘canonicalize-path’ calls, which are expensive (see the output of

‘strace’). This could become a problem if, for example, a channel has

many package definitions that refer to patches and auxiliary files via

‘local-file’.

Can this be avoided?

Another issue is that it changes the semantics of

‘current-source-directory’ in the presence of symlinks. That’s the

whole point, but I wonder if that’s always desirable (see below).

Below is another way to write this test:

1. Using ‘with-directory-excursion’ so the current directory is

switched back to what it was after this test.

2. Using ‘resolve-module’ instead of ‘use-modules’ (the latter should

only be used at the top level).

3. Tweaked the comments.

But… here we have:

/tmpdir

|

+--- test-local-file.scm

+--- content

+--- dir

|

+--- test-local-file.scm

To me, it’s not unreasonable for (local-file "content") to fail when

loading ‘dir/test-local-file.scm’. I would say that this is what most

people would expect.

So maybe we should go back to the actual use case and take a step back:

https://lists.gnu.org/archive/html/guix-devel/2024-08/msg00047.html

I didn’t hit this problem, presumably because my GUILE_LOAD_PATH does

not contain ‘~/.config/guix/current/share/guile/site/3.0’ (I use Guile

and Shepherd as channels¹).

Is there anything else we can do to address this?

Sorry for providing more questions that answers!

Thanks,

Ludo’.

¹ (append (list (channel

(name 'shepherd)

(url "https://git.savannah.gnu.org/git/shepherd.git")

(branch "devel")

(introduction

(make-channel-introduction

"788a6d6f1d5c170db68aa4bbfb77024fdc468ed3"

(openpgp-fingerprint

"3CE464558A84FDC69DB40CFB090B11993D9AEBB5"))))

(channel

(name 'guile)

(url "https://git.savannah.gnu.org/git/guile.git")

(branch "main")))

%default-channels)

Hello Ludo, in my opinion,

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

Yes, the present situation is that from the real file and the symlink,

one of them will not run.

Note that the test here is the converse situation of the guile channel

[1] described in the cookbook. There, the symlink is in the outer

directory and the real file in the inner directory, which refers to

(source (local-file "../.." "guile-checkout"

Regards,

Florian

My reply was lacking. I accidentally deleted footnote

[1] https://git.savannah.gnu.org/cgit/guile.git/tree/

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

Yes, exactly.

Does not the mlambda Ludo had put in absolute-dirname resolve all

canonicalize-path concerns? There are many patches, but all have the

same file.

Regards,

Florian