Support CLCACHE_BASEDIR in nodirect mode#349
Support CLCACHE_BASEDIR in nodirect mode#349oktal3700 wants to merge 1 commit intofrerich:masterfrom
Conversation
swift-project-old
force-pushed
the
basedir-nodirect
branch
2 times, most recently
from
15b347e to
33e5a0c
Compare
|
Not sure what caused the build to fail. |
frerich
left a comment
frerich
left a comment
There was a problem hiding this comment.
I think this looks like a plausible improvement, thanks a lot! However, it also invalidates all existing cache entries for people who do not use the direct mode, i.e. I guess this shouldn't be part of a bugfix release but rather of a feature release.
clcache/__main__.py
Outdated
| output += data[index:nextIndex] + new | ||
| index = nextIndex + len(old) | ||
|
|
||
|
|
There was a problem hiding this comment.
I'm not sure you need a dedicated function for this -- maybe it would be viable to just use re.sub here? I think caseInsensitiveReplace is basically
re.sub(re.escape(old), lambda _: new, data, flags=re.IGNORECASE)There was a problem hiding this comment.
Rightly or wrongly, for simple things I personally like to avoid regex for performance reasons. But I'm a beginner in Python so if you prefer regex here I'm happy to oblige. And re.escape should protect it from any shenanigans.
| normalizedCmdLine = CompilerArtifactsRepository._normalizedCommandLine(commandLine) | ||
|
|
||
| if "CLCACHE_BASEDIR" in os.environ: | ||
| baseDir = normalizeBaseDir(os.environ["CLCACHE_BASEDIR"]).replace("\\", "\\\\").encode("UTF-8") |
There was a problem hiding this comment.
Hmm, why are backslashes escaped here?
There was a problem hiding this comment.
It's to match the backslashes that are escaped in the preprocessor output because __FILE__ expands to a C string literal.
|
|
||
| return [arg for arg in cmdline | ||
| if not (arg[0] in "/-" and arg[1:].startswith(argsToStrip))] | ||
| if arg[0] in "/-" and not arg[1:].startswith(argsToStrip)] |
There was a problem hiding this comment.
Hmm, did you mean to change the behaviour here? I suspect the and should become or here. De Morgan found that not (a and b) is equivalent to not a or not b.
That being said, I'm not sure the new version is any nicer than the old one, so maybe it's not worth touching this at all.
There was a problem hiding this comment.
Oh, I only now noticed that you meant to fix a bug here. In that case, can you please add a test case which demonstrates the bug?
There was a problem hiding this comment.
testBasedirNoDirect would fail without this bugfix, but I can add a test that exercises only this bugfix and none of the other changes, if you like.
tests/test_unit.py
Outdated
| self.assertEqual(replace(b"infix paTTErn embed"), b"infix replacement embed") | ||
| self.assertEqual(replace(b"Pattern and PATTERN and pattern"), b"replacement and replacement and replacement") | ||
| self.assertEqual(replace(b"and finally PATTERN"), b"and finally replacement") | ||
|
|
There was a problem hiding this comment.
I suppose that in case you decide to drop the custom caseInsensitiveReplace function in favor of re.sub, you could also drop this test.
Are you sure? Only |
swift-project-old
force-pushed
the
basedir-nodirect
branch
from
33e5a0c to
587f132
Compare
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 88.93% 88.96% +0.03%
==========================================
Files 4 4
Lines 1301 1305 +4
Branches 195 196 +1
==========================================
+ Hits 1157 1161 +4
Misses 106 106
Partials 38 38
Continue to review full report at Codecov.
|
swift-project-old
force-pushed
the
basedir-nodirect
branch
from
587f132 to
c876277
Compare
|
Applied suggested changes. |
Do a simple case-insensitive find-and-replace to transform absolute paths into relative paths within the preprocessor output that is used to compute the hash. This makes CLCACHE_NODIRECT mode usable in the presence of the __FILE__ macro. Fixes a bug in _normalizedCommandLine that caused the source filename to be included in the hash computation.
swift-project-old
force-pushed
the
basedir-nodirect
branch
from
c876277 to
863fdec
Compare
3 participants
Do a simple case-insensitive find-and-replace to transform absolute
paths into relative paths within the preprocessor output that is used
to compute the hash. This makes CLCACHE_NODIRECT mode usable in the
presence of the
__FILE__macro.Fixes a bug in _normalizedCommandLine that caused the source filename
to be included in the hash computation.