Re: [ccache] base_dir and symbolic links

[Joel Rosdahl via ccache]

On 2 January 2018 at 13:49, Andreas Wettstein via ccache <
ccache@lists.samba.org> wrote:

> Hello Joel,
>
> > https://www.mail-archive.com/ccache@lists.samba.org/msg00802.html
>
> Thank you.  It is trickier than I thought.
>
> > ccache has a test suite and it fails like this with your patch:
>
> Sorry, I missed the test directory.  Attached is a modified patch, which
> fixes this issue and includes tests as well.
>

Thanks, but that unfortunately doesn't normalize "../" parts like the
current code with realpath does. For example, the following change to the
test suite makes it fail with your proposal:

--- a/test.sh
+++ b/test.sh
@@ -2355,8 +2355,8 @@ SUITE_basedir() {
 expect_stat 'cache miss' 1

 # Rewriting triggered by CCACHE_BASEDIR should handle paths with
multiple
-# slashes correctly:
-CCACHE_BASEDIR=`pwd` $CCACHE_COMPILE -I`pwd`//include -c
`pwd`//src/test.c
+# slashes, redundant "/." parts and "foo/.." parts correctly:
+CCACHE_BASEDIR=`pwd` $CCACHE_COMPILE -I`pwd`//./include/../include -c
`pwd`/src/test.c
 expect_stat 'cache hit (direct)' 1
 expect_stat 'cache hit (preprocessed)' 0
 expect_stat 'cache miss' 1


I would prefer not to lose that ability.

-- Joel
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache

Re: [ccache] base_dir and symbolic links

[Andreas Wettstein via ccache]

Hello Joel,

> https://www.mail-archive.com/ccache@lists.samba.org/msg00802.html

Thank you.  It is trickier than I thought.

> ccache has a test suite and it fails like this with your patch:

Sorry, I missed the test directory.  Attached is a modified patch, which
fixes this issue and includes tests as well.

> If I understand your setup correctly, would it be possible to instead put
> /reference, /user1 and /user2 under a /common directory and then let the
> users set basedir to /common?

Technically, yes, however, the file system layout is not under my
control.  It also would be tedious, as there are many more users, their
individual directories can be huge, and the ability to benefit from
ccache is just a minor issue in the overall picture.

Andreas

>From 8cf16d90cadebdf354c0cdd71f247af71941a413 Mon Sep 17 00:00:00 2001
From: Andreas Wettstein 
Date: Tue, 2 Jan 2018 13:37:02 +0100
Subject: [PATCH] When computing a relative path with respect to the current
 working directory, use the original path, without making it canonical.

Signed-off-by: Andreas Wettstein 
---
 ccache.c | 23 ---
 ccache.h |  1 +
 test.sh  | 10 ++
 test/test_util.c | 43 +++
 util.c   | 29 +
 5 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/ccache.c b/ccache.c
index edbc5cc..f8b8976 100644
--- a/ccache.c
+++ b/ccache.c
@@ -709,23 +709,16 @@ make_relative_path(char *path)
 		free(p);
 	}
 
-	char *canon_path = x_realpath(path);
-	if (canon_path) {
-		free(path);
-		char *relpath = get_relative_path(get_current_working_dir(), canon_path);
-		free(canon_path);
-		if (path_suffix) {
-			path = format("%s/%s", relpath, path_suffix);
-			free(relpath);
-			free(path_suffix);
-			return path;
-		} else {
-			return relpath;
-		}
-	} else {
-		// path doesn't exist, so leave it as it is.
+	remove_redunant_path_components(path);
+	char *relpath = get_relative_path(get_current_working_dir(), path);
+	free(path);
+	if (path_suffix) {
+		path = format("%s/%s", relpath, path_suffix);
+		free(relpath);
 		free(path_suffix);
 		return path;
+	} else {
+		return relpath;
 	}
 }
 
diff --git a/ccache.h b/ccache.h
index b282907..78e1eea 100644
--- a/ccache.h
+++ b/ccache.h
@@ -172,6 +172,7 @@ char *get_cwd(void);
 bool same_executable_name(const char *s1, const char *s2);
 size_t common_dir_prefix_length(const char *s1, const char *s2);
 char *get_relative_path(const char *from, const char *to);
+void remove_redunant_path_components(char *path);
 bool is_absolute_path(const char *path);
 bool is_full_path(const char *path);
 bool is_symlink(const char *path);
diff --git a/test.sh b/test.sh
index b3bdab3..9c7faec 100755
--- a/test.sh
+++ b/test.sh
@@ -2335,6 +2335,10 @@ int test;
 EOF
 cp -r dir1 dir2
 backdate dir1/include/test.h dir2/include/test.h
+
+mkdir dir3
+cp -R dir1/src dir3/src
+ln -s `pwd`/dir1/include dir3/include
 }
 
 SUITE_basedir() {
@@ -2384,6 +2388,12 @@ SUITE_basedir() {
 expect_stat 'cache hit (preprocessed)' 0
 expect_stat 'cache miss' 1
 
+cd ../dir3
+CCACHE_BASEDIR="`pwd`" $CCACHE_COMPILE -I`pwd`/include -c src/test.c
+expect_stat 'cache hit (direct)' 2
+expect_stat 'cache hit (preprocessed)' 0
+expect_stat 'cache miss' 1
+
 # -
 TEST "Rewriting in stderr"
 
diff --git a/test/test_util.c b/test/test_util.c
index dca718c..b367aae 100644
--- a/test/test_util.c
+++ b/test/test_util.c
@@ -85,6 +85,49 @@ TEST(get_relative_path)
 #endif
 }
 
+TEST(remove_redunant_path_components)
+{
+	char *str0 = strdup("");
+	remove_redunant_path_components(str0);
+	CHECK_STR_EQ_FREE2("", str0);
+
+	char *str1 = strdup("//");
+	remove_redunant_path_components(str1);
+	CHECK_STR_EQ_FREE2("/", str1);
+
+	char *str2 = strdup("x//.//");
+	remove_redunant_path_components(str2);
+	CHECK_STR_EQ_FREE2("x/", str2);
+
+	char *str3 = strdup("//x");
+	remove_redunant_path_components(str3);
+	CHECK_STR_EQ("/x", str3);
+
+	char *str4 = strdup("//x//.");
+	remove_redunant_path_components(str4);
+	CHECK_STR_EQ("/x/", str4);
+
+	char *str5 = strdup("//x//./");
+	remove_redunant_path_components(str5);
+	CHECK_STR_EQ("/x/", str5);
+
+	char *str6 = strdup("/x//..");
+	remove_redunant_path_components(str6);
+	CHECK_STR_EQ("/x/..", str6);
+
+	char *str7 = strdup("/");
+	remove_redunant_path_components(str7);
+	CHECK_STR_EQ_FREE2("/", str7);
+
+	char *str8 = strdup(".");
+	remove_redunant_path_components(str8);
+	CHECK_STR_EQ_FREE2("", str8);
+
+	char *str9 = strdup("./x");
+	remove_redunant_path_components(str9);
+	CHECK_STR_EQ_FREE2("x", str9);
+}
+
 TEST(format_hash_as_string)
 {
 	unsigned char hash[16] = {
diff --git a/util.c b/util.c
index e3889b6..af26ca8 100644
--- a/util.c
+++ b/util.c
@@ -1367,6 +1367,35 @@ get_relative_path(const char *from, const char *to)
 	return result;
 }
 
+// Removes re

Re: [ccache] base_dir and symbolic links

[Joel Rosdahl via ccache]

Hi Andreas,

> [...] I understand that it makes
> sense to make the current working directory canonic, but I do not
> see why this done for the path given to `make_relative_path`.  Is
> it really necessary?

Some background can be found in this mail thread:

*https://www.mail-archive.com/ccache@lists.samba.org/msg00802.html
*

The summary is that we want to normalize the paths to increase cache hits,
and one simple and robust way of doing that is to use realpath. Writing a
custom function that both normalizes paths and doesn't expand symlinks
might be possible, but note that there are tricky edge cases that need to
be handled.

> Thank you for considering it.  A patch is in the attachment.

ccache has a test suite and it fails like this with your patch:


Running test suite basedir...
FAILED

Test suite: basedir
Test case:  Path normalization
Failure reason: Expected "cache hit (direct)" to be 1, actual 0


[From your original mail:]
> For directories they do not work on, but are required to build the sources
> they work on, they create a symbolic link to a reference area in which
> only stuff required for building resides. [...] Now, assume user1 sets
> `base_dir` to `/user1` and user2 sets `base_dir` to `/user2`.

If I understand your setup correctly, would it be possible to instead put
/reference, /user1 and /user2 under a /common directory and then let the
users set basedir to /common?

-- Joel


On 17 September 2017 at 10:43, Andreas Wettstein via ccache <
ccache@lists.samba.org> wrote:

> > Seems like a pretty niche use case, but if you can come up with a
> > patch that doesn't break too much of current behaviour - why not.
>
> Thank you for considering it.  A patch is in the attachment.
>
> > Worst case, it could be an option (that defaulted to false) ?
>
> I would not consider this the worst case.  For me, it would be still
> more convenient than keeping my own modified version around.
>
> Andreas
>
>
> ___
> ccache mailing list
> ccache@lists.samba.org
> https://lists.samba.org/mailman/listinfo/ccache
>
>
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache

Re: [ccache] base_dir and symbolic links

[Andreas Wettstein via ccache]

> Seems like a pretty niche use case, but if you can come up with a
> patch that doesn't break too much of current behaviour - why not.

Thank you for considering it.  A patch is in the attachment.

> Worst case, it could be an option (that defaulted to false) ?

I would not consider this the worst case.  For me, it would be still
more convenient than keeping my own modified version around.

Andreas

>From b7acbbf1c61b137d10904ed8c4e3e2cbf27ddb6f Mon Sep 17 00:00:00 2001
From: Andreas Wettstein 
Date: Sat, 16 Sep 2017 20:24:27 +0200
Subject: [PATCH] When computing a relative path with respect to the current
 working directory, use the original path, without making it canonical.

Signed-off-by: Andreas Wettstein 
---
 ccache.c | 22 +++---
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/ccache.c b/ccache.c
index edbc5cc..2e605b3 100644
--- a/ccache.c
+++ b/ccache.c
@@ -709,23 +709,15 @@ make_relative_path(char *path)
 		free(p);
 	}
 
-	char *canon_path = x_realpath(path);
-	if (canon_path) {
-		free(path);
-		char *relpath = get_relative_path(get_current_working_dir(), canon_path);
-		free(canon_path);
-		if (path_suffix) {
-			path = format("%s/%s", relpath, path_suffix);
-			free(relpath);
-			free(path_suffix);
-			return path;
-		} else {
-			return relpath;
-		}
-	} else {
-		// path doesn't exist, so leave it as it is.
+	char *relpath = get_relative_path(get_current_working_dir(), path);
+	free(path);
+	if (path_suffix) {
+		path = format("%s/%s", relpath, path_suffix);
+		free(relpath);
 		free(path_suffix);
 		return path;
+	} else {
+		return relpath;
 	}
 }
 
-- 
2.14.1

___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache

Re: [ccache] base_dir and symbolic links

[Anders Björklund via ccache]

Andreas Wettstein wrote:
> In function `make_relative_path` (file `ccache.c`), the given
> path is made "canonic" before it is converted to a path relative
> to the current working directory.  In particular, "canonic" means
> that symbolic links are removed.  I understand that it makes
> sense to make the current working directory canonic, but I do not
> see why this done for the path given to `make_relative_path`.  Is
> it really necessary?
> 
> The reason why I am asking is that I have a usage scenario where
> this reduces sharing a cache among different users.

Seems like a pretty niche use case, but if you can come up with a
patch that doesn't break too much of current behaviour - why not.

You should still get preprocessor hits, but suppose you would
rather get "direct" hits instead of having to read both files.

Worst case, it could be an option (that defaulted to false) ?

Then you could override it at runtime, for your use case...

/Anders
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache

[ccache] base_dir and symbolic links

[Andreas Wettstein via ccache]

In function `make_relative_path` (file `ccache.c`), the given
path is made "canonic" before it is converted to a path relative
to the current working directory.  In particular, "canonic" means
that symbolic links are removed.  I understand that it makes
sense to make the current working directory canonic, but I do not
see why this done for the path given to `make_relative_path`.  Is
it really necessary?

The reason why I am asking is that I have a usage scenario where
this reduces sharing a cache among different users.

Much simplified, assume that we have a source code repository that
manages a couple of directories.  Individual users only check out the
directories they work on.  For directories they do not work on, but
are required to build the sources they work on, they create a symbolic
link to a reference area in which only stuff required for building
resides.  During compilation, the working directory is always one of
the source code directories.

For example, assume user1 is working in directory `lib` and
`src`, and user2 is working in `src` only.  Then the setup will
look like this:

/reference/
lib/
lib.h

/user1/
lib/
lib.h
lib.c
src/
test.c

/user1/
lib  -> /reference/lib
src/
test.c

Now, assume user1 sets `base_dir` to `/user1` and user2 sets
`base_dir` to `/user2`.  The working directories when compiling
`test.c` are `/user1/src` and `/user2/src`, respectively.  The paths
to the lib directory are `/user1/lib` and `/user2/lib`.  However, the
latter canonically becomes `/reference/lib`.  Therefore, the relative
paths become `../lib` and `../../referece/lib`, respectively. As the
relative paths are not equal, the cache cannot be shared, even if the
`lib.h` and `test.c` files are identical.  With the original paths
(not resolving the symbolic links), the relative paths to lib would be
`../lib` in either case, and the cache could be shared.

Regards,

Andreas

___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache