Re: [ccache] ccache problem with CCACHE_BASEDIR
On 2012-07-28 11:24, Joel Rosdahl wrote: Eric, I've now applied your patch with some tweaks in 2df269a3121889ebcdfa5d98dfb4d675f690e039 (see http://git.samba.org/?p=ccache.git;a=commit;h=2df269a3121889ebcdfa5d98dfb4d675f690e039). Thanks! Thanks for fixing this up the patch and applying it. I had meant to make the bool variable static in the original patch. I like your version better. Thanks, Eric ___ ccache mailing list ccache@lists.samba.org https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
On 27 July 2012 14:33, Joel Rosdahl wrote: > [...] So, the code needs to handle x_realpath returning NULL. Eric, I've now applied your patch with some tweaks in 2df269a3121889ebcdfa5d98dfb4d675f690e039 (see http://git.samba.org/?p=ccache.git;a=commit;h=2df269a3121889ebcdfa5d98dfb4d675f690e039). Thanks! > Thinking more about this, it would probably be better to (instead of > realpath) use some function that doesn't expand symlinks and doesn't > care whether the path exists or not. [...] On the other hand, such a function would still need to expand symlinks when there are ".." components in the path, which feels quite complex. -- Joel ___ ccache mailing list ccache@lists.samba.org https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
Hi, It seems you forgot to make cwd_canonicalized static? Currently, it has no effect. Also, the test suite crashes with your patch. I recommend running the test suite during development even for minor patches (and for bonus points: add new tests for the new functionality :-)). The crash happens because realpath returns NULL for nonexisting paths, and the test suite sets up fake paths for some parameters; this could happen in real usage as well. So, the code needs to handle x_realpath returning NULL. Thinking more about this, it would probably be better to (instead of realpath) use some function that doesn't expand symlinks and doesn't care whether the path exists or not. I'm not aware of such an existing (and portable) function, though, so we likely will have to write it ourselves. -- Joel On 20 July 2012 19:45, Eric Blau wrote: > On 2012-07-20 13:09, Joel Rosdahl wrote: >> >> Thanks. Two things: >> >> 1. The patch canonicalizes the "from" parameter, but I think that the >> "to" parameter should be canonicalized as well, for two reasons: a) >> realpath() expands symlinks, and comparing (see >> common_dir_prefix_length) an expanded path with an unexpanded path >> won't work well when symlinks are present. b) The "to" parameter >> (which for instance may be a path given on the command line) could >> also contain path elements that could benefit from canonicalization >> (to increase cache hits). >> >> 2. The patch canonicalizes the "from" parameter every time >> get_relative_path is called, which is unnecessary since it's always >> the same. Therefore, as mentioned earlier, I think that its' better to >> do the canonicalization in make_relative_path where >> current_working_dir can be canonicalized when created. > > > Thanks for the comments. Please discard the previous patch. This attached > version of the patch should be better. It canonicalizes the current working > directory once and canonicalizes the path each time get_relative_path is > called. > > Thanks, > Eric > > > ___ > 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] ccache problem with CCACHE_BASEDIR
On 2012-07-20 13:09, Joel Rosdahl wrote:
Thanks. Two things:
1. The patch canonicalizes the "from" parameter, but I think that the
"to" parameter should be canonicalized as well, for two reasons: a)
realpath() expands symlinks, and comparing (see
common_dir_prefix_length) an expanded path with an unexpanded path
won't work well when symlinks are present. b) The "to" parameter
(which for instance may be a path given on the command line) could
also contain path elements that could benefit from canonicalization
(to increase cache hits).
2. The patch canonicalizes the "from" parameter every time
get_relative_path is called, which is unnecessary since it's always
the same. Therefore, as mentioned earlier, I think that its' better to
do the canonicalization in make_relative_path where
current_working_dir can be canonicalized when created.
Thanks for the comments. Please discard the previous patch. This
attached version of the patch should be better. It canonicalizes the
current working directory once and canonicalizes the path each time
get_relative_path is called.
Thanks,
Eric
diff -uNr ccache-3.1.7/ccache.c ccache-3.1.7.patched/ccache.c
--- ccache-3.1.7/ccache.c 2012-01-08 09:40:55.0 -0500
+++ ccache-3.1.7.patched/ccache.c 2012-07-20 13:41:21.0 -0400
@@ -387,13 +387,31 @@
make_relative_path(char *path)
{
char *relpath;
+ char *canon_path;
+ char *canon_cwd;
+ bool cwd_canonicalized = false;
if (!base_dir || !str_startswith(path, base_dir)) {
return path;
}
- relpath = get_relative_path(current_working_dir, path);
+ /* Canonicalize the current working directory if it has not been
+* canonicalized already.
+*/
+ if (!cwd_canonicalized) {
+ canon_cwd = x_realpath(current_working_dir);
+ free(current_working_dir);
+
+ current_working_dir = canon_cwd;
+
+ cwd_canonicalized = true;
+ }
+
+ canon_path = x_realpath(path);
+
+ relpath = get_relative_path(current_working_dir, canon_path);
free(path);
+ free(canon_path);
return relpath;
}
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
On 19 July 2012 21:21, Eric Blau wrote: > Sorry for the slow response, but I have a patch attached for this issue. > Please let me know if you have any comments. Thanks. Two things: 1. The patch canonicalizes the "from" parameter, but I think that the "to" parameter should be canonicalized as well, for two reasons: a) realpath() expands symlinks, and comparing (see common_dir_prefix_length) an expanded path with an unexpanded path won't work well when symlinks are present. b) The "to" parameter (which for instance may be a path given on the command line) could also contain path elements that could benefit from canonicalization (to increase cache hits). 2. The patch canonicalizes the "from" parameter every time get_relative_path is called, which is unnecessary since it's always the same. Therefore, as mentioned earlier, I think that its' better to do the canonicalization in make_relative_path where current_working_dir can be canonicalized when created. -- Joel ___ ccache mailing list ccache@lists.samba.org https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
On 2012-06-11 07:54, Eric Blau wrote:
On 2012-06-10 16:09, Joel Rosdahl wrote:
> My suggestion is that get_relative_path essentially should stay the
> same and continue to assume that the paths are well-formed and
> canonical. make_relative_path (the only caller of get_relative_path)
> should canonicalize current_working_dir and path by calling
> realpath(3). Does this sound reasonble?
Thanks for looking at this, Joel. I modified the code to do essentially
this and it is working great now. I'll send a patch to you and the list
shortly.
Hi Joel,
Sorry for the slow response, but I have a patch attached for this issue.
Please let me know if you have any comments.
Thanks,
Eric
diff -uNr ccache-3.1.7/util.c ccache-3.1.7.patched/util.c
--- ccache-3.1.7/util.c 2012-01-08 09:40:55.0 -0500
+++ ccache-3.1.7.patched/util.c 2012-07-19 12:41:37.0 -0400
@@ -993,11 +993,15 @@
int i;
const char *p;
char *result;
+ char *cpath;
if (!*to || *to != '/') {
return x_strdup(to);
}
+ cpath = x_realpath(from);
+ from = cpath;
+
result = x_strdup("");
common_prefix_len = common_dir_prefix_length(from, to);
for (p = from + common_prefix_len; *p; p++) {
@@ -1021,6 +1025,7 @@
free(result);
result = x_strdup(".");
}
+ free(cpath);
return result;
}
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
On 2012-06-10 16:09, Joel Rosdahl wrote: My suggestion is that get_relative_path essentially should stay the same and continue to assume that the paths are well-formed and canonical. make_relative_path (the only caller of get_relative_path) should canonicalize current_working_dir and path by calling realpath(3). Does this sound reasonble? Thanks for looking at this, Joel. I modified the code to do essentially this and it is working great now. I'll send a patch to you and the list shortly. Thanks, Eric ___ ccache mailing list ccache@lists.samba.org https://lists.samba.org/mailman/listinfo/ccache
Re: [ccache] ccache problem with CCACHE_BASEDIR
On 5 June 2012 17:47, Blau, Eric wrote: > [...] I'm seeing ccache do inappropriate replacements of absolute paths with > relative paths. [...] This code is not considering if there is a "/./" in the > absolute path, but is blindly replacing this with "..". Right, that's a bug; the code is assuming too much of the input. Thanks for reporting this! > Would a patch be accepted for this problem if I fixed it? Yes, a patch would certainly be welcome if you like to work on it. > Any other suggestions? My suggestion is that get_relative_path essentially should stay the same and continue to assume that the paths are well-formed and canonical. make_relative_path (the only caller of get_relative_path) should canonicalize current_working_dir and path by calling realpath(3). Does this sound reasonble? -- Joel ___ ccache mailing list ccache@lists.samba.org https://lists.samba.org/mailman/listinfo/ccache
[ccache] ccache problem with CCACHE_BASEDIR
Hi folks,
I've been trying to use ccache with the CCACHE_BASEDIR option so that my fellow
team members can take advantage of compiles that have already been done by each
other from a different directory. I'm seeing ccache do inappropriate
replacements of absolute paths with relative paths. Here is an example:
I have CCACHE_BASEDIR set to "/opt/eblau/comcol/Tcm6" and I'm trying to compile
from within a sub directory under that path. I have a directory with a bunch of
header files in "/opt/eblau/comcol/Tcm6/prod/include". When I do a build under,
for example, "/opt/eblau/comcol/Tcm6/src/core/comm/mx" I see this in the ccache
log:
[2012-06-05T11:33:20.022515 21600] Working directory:
/opt/eblau/comcol/Tcm6/src/./core/comm/mx
[2012-06-05T11:33:20.022520 21600] Base directory: /opt/eblau/comcol/Tcm6
[2012-06-05T11:33:20.022525 21600] Unify mode enabled
[2012-06-05T11:33:20.022530 21600] Direct mode disabled
[2012-06-05T11:33:20.022551 21600] -g used; disabling unify mode
[2012-06-05T11:33:20.022582 21600]
dirname=/opt/eblau/comcol/Tcm6/src/./core/comm
[2012-06-05T11:33:20.022594 21600] full
path=/opt/eblau/comcol/Tcm6/src/./core/comm/mx
[2012-06-05T11:33:20.022620 21600] Source file: MxServerLink.cxx
[2012-06-05T11:33:20.022625 21600] Object file: MxServerLink.o
[2012-06-05T11:33:20.022636 21600] Running preprocessor
[2012-06-05T11:33:20.022655 21600] Executing /usr/bin/g++ -c -fno-exceptions
-Wall -Wno-deprecated -Wno-trigraphs -Wno-strict-aliasing -march=x86-64 -g
-Dlinux -D_GNU_SOURCE -D__STDC_LIMIT_MACROS -Wno-invalid-offsetof -D_REENTRANT
-fPIC -I. -I../../../../../prod/include -I/usr/src/LiS/include
-I/usr/lib/g++-include -I/usr/lib/gcc/x86_64-redhat-linux/4.1.2/include -E
MxServerLink.cxx
[2012-06-05T11:33:20.022729 21601] Unlink
/opt/cache/ccache/tmp/MxServerLi.tmp.cmdev3.21600.ii (as-tmp)
[2012-06-05T11:33:20.022815 21601] Unlink
/opt/cache/ccache/tmp/tmp.cpp_stderr.cmdev3.21600 (as-tmp)
[2012-06-05T11:33:20.058076 21600] Unlink
/opt/cache/ccache/tmp/MxServerLi.tmp.cmdev3.21600.ii (as-tmp)
[2012-06-05T11:33:20.058232 21600] Unlink
/opt/cache/ccache/tmp/tmp.cpp_stderr.cmdev3.21600 (as-tmp)
[2012-06-05T11:33:20.058257 21600] Preprocessor gave exit status 1
[2012-06-05T11:33:20.058266 21600] Failed; falling back to running the real
compiler
ccache is misinterpreting the "." in the absolute path and adding on extra ".."
to the -I path in the compile, causing it to fail to find the header header
files under "/opt/eblau/comcol/Tcm6/prod/include".
The buggy code looks to be in get_relative_path() in util.c:
common_prefix_len = common_dir_prefix_length(from, to);
for (p = from + common_prefix_len; *p; p++) {
if (*p == '/') {
x_asprintf2(&result, "../%s", result);
}
}
This code is not considering if there is a "/./" in the absolute path, but is
blindly replacing this with "..". Has anyone else hit this? Would a patch be
accepted for this problem if I fixed it? Any other suggestions?
Thanks,
Eric
___
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache
