| Message ID | 20170706104841.7711-1-peter@korsgaard.com |
|---|---|
| State | Accepted |
| Commit | bdca0d05816fec8472b1d32301f55df152b86466 |
| Headers | show |
Hello, On Thu, 6 Jul 2017 12:48:41 +0200, Peter Korsgaard wrote: > While building I noticed: > > >>> host-ccache 3.3.4 Building > conf.c: In function 'conf_create': > conf.c:314:2: warning: too many arguments for format [-Wformat-extra-args] > conf->cache_dir = format("/home/peko/.buildroot-ccache", get_home_directory()); > ^ > > As host-ccache gets installed into $(HOST_DIR) and is part of the SDK, > hardcoding the build user homedir isn't really nice for the relocatable > SDK feature (or simply for a SDK used by multiple users). > > As the warning shows, CCache replaces "%s" with the current user home > directory, so rewrite BR_CACHE_DIR to use this feature if it begins with > $HOME. > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > package/ccache/ccache.mk | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/package/ccache/ccache.mk b/package/ccache/ccache.mk > index 97d66bb45b..afbec44fac 100644 > --- a/package/ccache/ccache.mk > +++ b/package/ccache/ccache.mk > @@ -28,9 +28,13 @@ HOST_CCACHE_CONF_OPTS += --with-bundled-zlib > # BR2_CCACHE_DIR. > # - Change hard-coded last-ditch default to match path in .config, to avoid > # the need to specify BR_CACHE_DIR when invoking ccache directly. > +# CCache replaces "%s" with the home directory of the current user, > +# So rewrite BR_CACHE_DIR to take that into consideration for SDK purpose > +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR)) But this only solves the problem for the specific case where the ccache cache is inside the same folder in user A home directory and user B home directory. If I set BR2_CCACHE_DIR to /home/thomas/projects/foobar/ccache/, which works on my machine, you most likely won't have /home/jacmet/projects/foobar/ccache/ on your machine. So I have the feeling that this only solves the problem for the specific case where the ccache cache is directly in a sub-directory of the home folder. Any other situation will continue to fail, and will anyway require a different solution, that would also fix the specific home folder case. Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR)) > But this only solves the problem for the specific case where the ccache > cache is inside the same folder in user A home directory and user B > home directory. > If I set BR2_CCACHE_DIR to /home/thomas/projects/foobar/ccache/, which > works on my machine, you most likely won't > have /home/jacmet/projects/foobar/ccache/ on your machine. No, but as far as I read the ccache code, it will create the cache dir + parent directories if they don't exist. E.G.: CCACHE_DIR=/tmp/some/funky/sub/directory ccache -s cache directory /tmp/some/funky/sub/directory primary config /tmp/some/funky/sub/directory/ccache.conf secondary config (readonly) /etc/ccache.conf cache hit (direct) 0 cache hit (preprocessed) 0 cache miss 0 cache hit rate 0.00 % cleanups performed 0 files in cache 0 cache size 0.0 kB max cache size 5.0 GB ls -l /tmp/some/funky/sub/directory total 4 -rw-r--r-- 1 peko peko 16 Jul 7 00:26 ccache.conf Hardcoding /home/thomas/.buildroot-ccache is very unlikely to work for other users, whereas ~/.buildroot-cache is quite likely.
On 06-07-17 12:48, Peter Korsgaard wrote: > While building I noticed: > >>>> host-ccache 3.3.4 Building > conf.c: In function 'conf_create': > conf.c:314:2: warning: too many arguments for format [-Wformat-extra-args] > conf->cache_dir = format("/home/peko/.buildroot-ccache", get_home_directory()); > ^ > > As host-ccache gets installed into $(HOST_DIR) and is part of the SDK, > hardcoding the build user homedir isn't really nice for the relocatable > SDK feature (or simply for a SDK used by multiple users). Absolutely true. However, I wonder if hardcoding the ccache path in the ccache binary is really such a good idea in the context of an SDK. So perhaps we should just replace the default with our own default, i.e. sed s,%s/\.ccache,%s/.buildroot-ccache, Note also that it may have been intentional to point to a shared ccache directory that happens to reside in some user's home directory. But of course, in that case, the BR_CACHE_DIR we use will still take over. > > As the warning shows, CCache replaces "%s" with the current user home > directory, so rewrite BR_CACHE_DIR to use this feature if it begins with > $HOME. > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > package/ccache/ccache.mk | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/package/ccache/ccache.mk b/package/ccache/ccache.mk > index 97d66bb45b..afbec44fac 100644 > --- a/package/ccache/ccache.mk > +++ b/package/ccache/ccache.mk > @@ -28,9 +28,13 @@ HOST_CCACHE_CONF_OPTS += --with-bundled-zlib > # BR2_CCACHE_DIR. > # - Change hard-coded last-ditch default to match path in .config, to avoid > # the need to specify BR_CACHE_DIR when invoking ccache directly. > +# CCache replaces "%s" with the home directory of the current user, > +# So rewrite BR_CACHE_DIR to take that into consideration for SDK purpose > +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR)) This looked really really weird to me when I first saw it, but in the end it does make sense. Still, since nobody ever complained about this, I'm not so sure we should "fix" it. It's just as likely to actually break things for some user. So it's a weak NACK from me. Regards, Arnout > + > define HOST_CCACHE_PATCH_CONFIGURATION > sed -i 's,getenv("CCACHE_DIR"),getenv("BR_CACHE_DIR"),' $(@D)/ccache.c > - sed -i 's,"%s/.ccache","$(BR_CACHE_DIR)",' $(@D)/conf.c > + sed -i 's,"%s/.ccache","$(HOST_CCACHE_DEFAULT_CCACHE_DIR)",' $(@D)/conf.c > endef > > HOST_CCACHE_POST_PATCH_HOOKS += HOST_CCACHE_PATCH_CONFIGURATION >
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: > On 06-07-17 12:48, Peter Korsgaard wrote: >> While building I noticed: >> >>>>> host-ccache 3.3.4 Building >> conf.c: In function 'conf_create': >> conf.c:314:2: warning: too many arguments for format [-Wformat-extra-args] conf-> cache_dir = format("/home/peko/.buildroot-ccache", get_home_directory()); >> ^ >> >> As host-ccache gets installed into $(HOST_DIR) and is part of the SDK, >> hardcoding the build user homedir isn't really nice for the relocatable >> SDK feature (or simply for a SDK used by multiple users). > Absolutely true. However, I wonder if hardcoding the ccache path in the ccache > binary is really such a good idea in the context of an SDK. So perhaps we should > just replace the default with our own default, i.e. > sed s,%s/\.ccache,%s/.buildroot-ccache, That is effectively what the patch does by default, while still allowing you to hardcode an absolute path or a ~/ relative one. We cannot easily remove the cache dir configuration option / default fixup as we have supported this for more than 3 years now - Since: commit d93a0b402934309c632d4a825b7fe6183ce8c4c7 Author: Danomi Manchego <danomimanchego123@gmail.com> Date: Wed Apr 30 22:05:06 2014 -0400 ccache: change default cache directory path to match config setting > Note also that it may have been intentional to point to a shared ccache > directory that happens to reside in some user's home directory. But of course, > in that case, the BR_CACHE_DIR we use will still take over. Correct, that chould possibly be a use case, but I would argue a quite unlikely one, whereas "I just enabled ccache in the sdk and now everything breaks horribly for my colleagues" is more likely. >> # - Change hard-coded last-ditch default to match path in .config, to avoid >> # the need to specify BR_CACHE_DIR when invoking ccache directly. >> +# CCache replaces "%s" with the home directory of the current user, >> +# So rewrite BR_CACHE_DIR to take that into consideration for SDK purpose >> +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR)) > This looked really really weird to me when I first saw it, but in the end it > does make sense. I agree that it isn't really obvious (which is why I added the comments above), but I don't see how we can otherwise fix it as BR_CACHE_DIR is already expanded by the time we get here. > Still, since nobody ever complained about this, I'm not so sure we should "fix" > it. It's just as likely to actually break things for some user. So it's a weak > NACK from me. We are only now adding relocatable SDK support, so I would imagine reuse of the $(HOST_DIR) between users & machines to grow in the future. I don't agree with the "just as likely to actually break" statement. CCache is already an advanced option. Using CCache and customizing the cache dir to a fixed path that happens to reside under the home directory of the user building it (on the build machine) is IMHO significantly less unlikely than just enabling ccache and not tweaking anything else. And even if so, for those advanced use cases you can always set BR_CACHE_DIR in the environment. Like for other features, we should make sure that simple things work out of the box and that advanced things are possible - But not that you should jump through extra hoops just to get ccache to work.
Hello, On Fri, 07 Jul 2017 00:28:49 +0200, Peter Korsgaard wrote: > > If I set BR2_CCACHE_DIR to /home/thomas/projects/foobar/ccache/, which > > works on my machine, you most likely won't > > have /home/jacmet/projects/foobar/ccache/ on your machine. > > No, but as far as I read the ccache code, it will create the cache dir + > parent directories if they don't exist. > > E.G.: > > CCACHE_DIR=/tmp/some/funky/sub/directory ccache -s > cache directory /tmp/some/funky/sub/directory > primary config /tmp/some/funky/sub/directory/ccache.conf > secondary config (readonly) /etc/ccache.conf > cache hit (direct) 0 > cache hit (preprocessed) 0 > cache miss 0 > cache hit rate 0.00 % > cleanups performed 0 > files in cache 0 > cache size 0.0 kB > max cache size 5.0 GB > ls -l /tmp/some/funky/sub/directory > total 4 > -rw-r--r-- 1 peko peko 16 Jul 7 00:26 ccache.conf OK, but I definitely don't want ccache to create a random directory somewhere in my $HOME folder, just because it was hardcoded into the ccache binary. > Hardcoding /home/thomas/.buildroot-ccache is very unlikely to work for > other users, whereas ~/.buildroot-cache is quite likely. Sure. But if I set BR2_CCACHE_DIR to /opt/ccache/, it won't work when the SDK is moved to another machine (permissions may not be set), and if I set BR2_CCACHE_DIR to $HOME/projects/foobar/cache/, and you run my SDK on your system, you will get this funky folder created. Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> No, but as far as I read the ccache code, it will create the cache dir + >> parent directories if they don't exist. >> >> E.G.: >> >> CCACHE_DIR=/tmp/some/funky/sub/directory ccache -s >> cache directory /tmp/some/funky/sub/directory >> primary config /tmp/some/funky/sub/directory/ccache.conf >> secondary config (readonly) /etc/ccache.conf >> cache hit (direct) 0 >> cache hit (preprocessed) 0 >> cache miss 0 >> cache hit rate 0.00 % >> cleanups performed 0 >> files in cache 0 >> cache size 0.0 kB >> max cache size 5.0 GB >> ls -l /tmp/some/funky/sub/directory >> total 4 >> -rw-r--r-- 1 peko peko 16 Jul 7 00:26 ccache.conf > OK, but I definitely don't want ccache to create a random directory > somewhere in my $HOME folder, just because it was hardcoded into the > ccache binary. Ehh, that is what ccache normally does (E.G. ~/.ccache), similar to a bunch of other utilities. Without this change things will just break instead as you most likely don't have a /home/peko/.buildroot-ccache on your system (or if you do, have write access to it). >> Hardcoding /home/thomas/.buildroot-ccache is very unlikely to work for >> other users, whereas ~/.buildroot-cache is quite likely. > Sure. But if I set BR2_CCACHE_DIR to /opt/ccache/, it won't work when > the SDK is moved to another machine (permissions may not be set), and > if I set BR2_CCACHE_DIR to $HOME/projects/foobar/cache/, and you run my > SDK on your system, you will get this funky folder created. You presumably would have specific reasons to customize BR2_CCACHE_DIR like this (and make the needed arrangements to have E.G /opt/ccache available), so I don't see what the problem would be. Having a BR2_CCACHE_DIR option (with a default value of $(HOME)/.buildroot-cache) ending up hardcoding /home/$sdkbuilduser/.buildroot-cache in the binary seems to me far from obvious behavior and goes against the goals of reproducible builds and reusable sdks.
On 07-07-17 11:48, Peter Korsgaard wrote: >>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > > Hi, > > >> No, but as far as I read the ccache code, it will create the cache dir + > >> parent directories if they don't exist. > >> > >> E.G.: > >> > >> CCACHE_DIR=/tmp/some/funky/sub/directory ccache -s > >> cache directory /tmp/some/funky/sub/directory > >> primary config /tmp/some/funky/sub/directory/ccache.conf > >> secondary config (readonly) /etc/ccache.conf > >> cache hit (direct) 0 > >> cache hit (preprocessed) 0 > >> cache miss 0 > >> cache hit rate 0.00 % > >> cleanups performed 0 > >> files in cache 0 > >> cache size 0.0 kB > >> max cache size 5.0 GB > >> ls -l /tmp/some/funky/sub/directory > >> total 4 > >> -rw-r--r-- 1 peko peko 16 Jul 7 00:26 ccache.conf > > > OK, but I definitely don't want ccache to create a random directory > > somewhere in my $HOME folder, just because it was hardcoded into the > > ccache binary. > > Ehh, that is what ccache normally does (E.G. ~/.ccache), similar to a > bunch of other utilities. Without this change things will just break > instead as you most likely don't have a /home/peko/.buildroot-ccache on > your system (or if you do, have write access to it). I think Thomas's point is: ~/.br-ccache is fine, ~/foo/bar/directory is not fine, even if that is what was configured in the buildroot config from which the external toolchain was generated (and which is NOT the buildroot configuration used by the final user, otherwise this problem wouldn't even occur). > >> Hardcoding /home/thomas/.buildroot-ccache is very unlikely to work for > >> other users, whereas ~/.buildroot-cache is quite likely. > > > Sure. But if I set BR2_CCACHE_DIR to /opt/ccache/, it won't work when > > the SDK is moved to another machine (permissions may not be set), and > > if I set BR2_CCACHE_DIR to $HOME/projects/foobar/cache/, and you run my > > SDK on your system, you will get this funky folder created. > > You presumably would have specific reasons to customize BR2_CCACHE_DIR > like this (and make the needed arrangements to have E.G /opt/ccache > available), so I don't see what the problem would be. > > Having a BR2_CCACHE_DIR option (with a default value of > $(HOME)/.buildroot-cache) ending up hardcoding > /home/$sdkbuilduser/.buildroot-cache in the binary seems to me far from > obvious behavior and goes against the goals of reproducible builds and > reusable sdks. I think Thomas, like me, means that we should instead revert d93a0b402934309c632d4a825b7fe6183ce8c4c7. In fact, I think we should drop this Buildroot-specific patching entirely. As far as I can see, BR_CACHE_DIR was introduced to replace the normal CCACHE_DIR because way back when our ccache didn't reliably detect the compiler and would reuse object files that we actually created with a different toolchain. So the Buildroot-specific ccache would disable fingerprinting completely. Obviously, if it would then reuse the same cache directory as the normal ccache, there would be a huge risk that target ccache would use cached objects for the host... Nowadays we have the fingerprinting in place (which is hopefully still reliable), so the reason for have a separate cache dir is gone IMO. So I think we should just use CCACHE_DIR instead of BR_CACHE_DIR. BR2_CCACHE_DIR still makes sense, since you may want to encode a shared ccache in your Buildroot config. But I think it should default to empty, where empty means don't export CCACHE_DIR and rely on ccache's default. Such a change, however, would first require a test to be set up. And testing ccache is decidedly non-trivial. Therefore it's a more long-term goal. Therefore, this patch makes sense after all. Indeed, if a user currently generates an SDK with the default configuration, it will definitely do the wrong thing (pointing to another user's br-ccache directory). So I'll change my weak NACK into a weak ACK. Regards, Arnout
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, > I think Thomas's point is: ~/.br-ccache is fine, ~/foo/bar/directory is not > fine, even if that is what was configured in the buildroot config from which the > external toolchain was generated (and which is NOT the buildroot configuration > used by the final user, otherwise this problem wouldn't even occur). Well, it is a choice made by whoever built the sdk, just like the gcc version and C library or whatever. If it truly makes sense to make this configuable is another question. I have never used that feature at least (but we have plenty of other features that I also don't use). > I think Thomas, like me, means that we should instead revert > d93a0b402934309c632d4a825b7fe6183ce8c4c7. That is also an option, but as we have been carrying this patch for > 3 years, there might be people using it. > In fact, I think we should drop this Buildroot-specific patching entirely. As > far as I can see, BR_CACHE_DIR was introduced to replace the normal CCACHE_DIR > because way back when our ccache didn't reliably detect the compiler and would > reuse object files that we actually created with a different toolchain. So the > Buildroot-specific ccache would disable fingerprinting completely. Obviously, if > it would then reuse the same cache directory as the normal ccache, there would > be a huge risk that target ccache would use cached objects for the host... > Nowadays we have the fingerprinting in place (which is hopefully still > reliable), so the reason for have a separate cache dir is gone IMO. So I think > we should just use CCACHE_DIR instead of BR_CACHE_DIR. BR2_CCACHE_DIR still > makes sense, since you may want to encode a shared ccache in your Buildroot > config. But I think it should default to empty, where empty means don't export > CCACHE_DIR and rely on ccache's default. Yes, I have also always been uneasy about these ccache patches - But I normally don't use ccache, so I'm not sure how bullet proof the finger printing is. > Such a change, however, would first require a test to be set up. And testing > ccache is decidedly non-trivial. Therefore it's a more long-term goal. > Therefore, this patch makes sense after all. Indeed, if a user currently > generates an SDK with the default configuration, it will definitely do the wrong > thing (pointing to another user's br-ccache directory). So I'll change my weak > NACK into a weak ACK. Ok, thanks.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: > While building I noticed: >>>> host-ccache 3.3.4 Building > conf.c: In function 'conf_create': > conf.c:314:2: warning: too many arguments for format [-Wformat-extra-args] conf-> cache_dir = format("/home/peko/.buildroot-ccache", get_home_directory()); > ^ > As host-ccache gets installed into $(HOST_DIR) and is part of the SDK, > hardcoding the build user homedir isn't really nice for the relocatable > SDK feature (or simply for a SDK used by multiple users). > As the warning shows, CCache replaces "%s" with the current user home > directory, so rewrite BR_CACHE_DIR to use this feature if it begins with > $HOME. > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> Committed, thanks.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: > While building I noticed: >>>> host-ccache 3.3.4 Building > conf.c: In function 'conf_create': > conf.c:314:2: warning: too many arguments for format [-Wformat-extra-args] conf-> cache_dir = format("/home/peko/.buildroot-ccache", get_home_directory()); > ^ > As host-ccache gets installed into $(HOST_DIR) and is part of the SDK, > hardcoding the build user homedir isn't really nice for the relocatable > SDK feature (or simply for a SDK used by multiple users). > As the warning shows, CCache replaces "%s" with the current user home > directory, so rewrite BR_CACHE_DIR to use this feature if it begins with > $HOME. > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> Committed to 2017.02.x and 2017.05.x, thanks.
diff --git a/package/ccache/ccache.mk b/package/ccache/ccache.mk index 97d66bb45b..afbec44fac 100644 --- a/package/ccache/ccache.mk +++ b/package/ccache/ccache.mk @@ -28,9 +28,13 @@ HOST_CCACHE_CONF_OPTS += --with-bundled-zlib # BR2_CCACHE_DIR. # - Change hard-coded last-ditch default to match path in .config, to avoid # the need to specify BR_CACHE_DIR when invoking ccache directly. +# CCache replaces "%s" with the home directory of the current user, +# So rewrite BR_CACHE_DIR to take that into consideration for SDK purpose +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR)) + define HOST_CCACHE_PATCH_CONFIGURATION sed -i 's,getenv("CCACHE_DIR"),getenv("BR_CACHE_DIR"),' $(@D)/ccache.c - sed -i 's,"%s/.ccache","$(BR_CACHE_DIR)",' $(@D)/conf.c + sed -i 's,"%s/.ccache","$(HOST_CCACHE_DEFAULT_CCACHE_DIR)",' $(@D)/conf.c endef HOST_CCACHE_POST_PATCH_HOOKS += HOST_CCACHE_PATCH_CONFIGURATION