-
Notifications
You must be signed in to change notification settings - Fork 83
Handle base dir in get manifest hash #238
Changes from all commits
52fe08c
4ec76a1
4b3cdd9
ea01aef
69dfb04
7d45cb3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,8 +97,8 @@ def childDirectories(path, absolute=True): | |
| def normalizeBaseDir(baseDir): | ||
| if baseDir: | ||
| baseDir = os.path.normcase(baseDir) | ||
| if not baseDir.endswith(os.path.sep): | ||
| baseDir += os.path.sep | ||
| if baseDir.endswith(os.path.sep): | ||
| baseDir = baseDir[0:-1] | ||
| return baseDir | ||
| else: | ||
| # Converts empty string to None | ||
|
|
@@ -227,11 +227,25 @@ def getManifestHash(compilerBinary, commandLine, sourceFile): | |
| compilerHash = getCompilerHash(compilerBinary) | ||
|
|
||
| # NOTE: We intentionally do not normalize command line to include | ||
| # preprocessor options. In direct mode we do not perform | ||
| # preprocessing before cache lookup, so all parameters are important. | ||
| # One of the few exceptions to this rule is the /MP switch, which only | ||
| # defines how many compiler processes are running simultaneusly. | ||
| # preprocessor options. In direct mode we do not perform preprocessing | ||
| # before cache lookup, so all parameters are important. One of the few | ||
| # exceptions to this rule is the /MP switch, which only defines how many | ||
| # compiler processes are running simultaneusly. Arguments that specify | ||
| # the compiler where to find the source files are parsed to replace | ||
| # ocurrences of CLCACHE_BASEDIR by a placeholder. | ||
| commandLine = [arg for arg in commandLine if not arg.startswith("/MP")] | ||
| arguments, inputFiles = CommandLineAnalyzer.parseArgumentsAndInputFiles(commandLine) | ||
| collapseBasedirInCmdPath = lambda path: collapseBasedirToPlaceholder(os.path.normcase(os.path.abspath(path))) | ||
|
|
||
| commandLine = [] | ||
| argumentsWithPaths = ("AI", "I", "FU") | ||
| for k in sorted(arguments.keys()): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the sorted is very important to have repeatable manifest hashes. As dictionaries in python are unordered the only way to guarantee that we generate the same hash every invocation is to iterate over the keys in the same order. It is the same case that #235 solved.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so this basically improves the manifest cache hit rate, i.e. it's an improvement but not directly related to the BASEDIR feature? Ok.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is going to have that side effect but at same time it is needed for correctness since the implementation changed in this PR to use a dictionary. |
||
| if k in argumentsWithPaths: | ||
| commandLine.extend(["/" + k + collapseBasedirInCmdPath(arg) for arg in arguments[k]]) | ||
| else: | ||
| commandLine.extend(["/" + k + arg for arg in arguments[k]]) | ||
|
|
||
| commandLine.extend(collapseBasedirInCmdPath(arg) for arg in inputFiles) | ||
|
|
||
| additionalData = "{}|{}|{}".format( | ||
| compilerHash, commandLine, ManifestRepository.MANIFEST_FILE_FORMAT_VERSION) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ | |
|
|
||
| PYTHON_BINARY = sys.executable | ||
| CLCACHE_SCRIPT = os.path.join(os.path.dirname(os.path.realpath(__file__)), "clcache.py") | ||
| ASSETS_DIR = os.path.join("tests", "integrationtests") | ||
| ASSETS_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), "tests", "integrationtests") | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a suspicion, but I'm not quite sure - why did you change this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was changed only to simplify the code in TestBasedir. The cwd is changed with
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, makes sense! 👍 |
||
|
|
||
| # pytest-cov note: subprocesses are coverage tested by default with some limitations | ||
| # "For subprocess measurement environment variables must make it from the main process to the | ||
|
|
@@ -1073,37 +1073,102 @@ def testHitsViaMpConcurrent(self): | |
|
|
||
|
|
||
| class TestBasedir(unittest.TestCase): | ||
| def testBasedir(self): | ||
| with cd(os.path.join(ASSETS_DIR, "basedir")), tempfile.TemporaryDirectory() as tempDir: | ||
| # First, create two separate build directories with the same sources | ||
| for buildDir in ["builddir_a", "builddir_b"]: | ||
| shutil.rmtree(buildDir, ignore_errors=True) | ||
| os.mkdir(buildDir) | ||
| def setUp(self): | ||
| self.projectDir = os.path.join(ASSETS_DIR, "basedir") | ||
| self.tempDir = tempfile.TemporaryDirectory() | ||
| self.clcacheDir = os.path.join(self.tempDir.name, "clcache") | ||
| self.savedCwd = os.getcwd() | ||
|
|
||
| os.chdir(self.tempDir.name) | ||
|
|
||
| # First, create two separate build directories with the same sources | ||
| for buildDir in ["builddir_a", "builddir_b"]: | ||
| shutil.copytree(self.projectDir, buildDir) | ||
|
|
||
| self.cache = clcache.Cache(self.clcacheDir) | ||
|
|
||
| def tearDown(self): | ||
| os.chdir(self.savedCwd) | ||
| self.tempDir.cleanup() | ||
|
|
||
| def _runCompiler(self, cppFile, extraArgs=None): | ||
| cmd = CLCACHE_CMD + ["/nologo", "/EHsc", "/c"] | ||
| if extraArgs: | ||
| cmd.extend(extraArgs) | ||
| cmd = cmd + [cppFile] | ||
| env = dict(os.environ, CLCACHE_DIR=self.clcacheDir, CLCACHE_BASEDIR=os.getcwd()) | ||
| self.assertEqual(subprocess.call(cmd, env=env), 0) | ||
|
|
||
| def expectHit(self, runCompiler): | ||
| # Build once in one directory | ||
| with cd("builddir_a"): | ||
| runCompiler[0]() | ||
| with self.cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 1) | ||
| self.assertEqual(stats.numCacheHits(), 0) | ||
|
|
||
| shutil.copy("main.cpp", buildDir) | ||
| shutil.copy("constants.h", buildDir) | ||
| # Build again in a different directory, this should hit now because of CLCACHE_BASEDIR | ||
| with cd("builddir_b"): | ||
| runCompiler[1]() | ||
| with self.cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 1) | ||
| self.assertEqual(stats.numCacheHits(), 1) | ||
|
|
||
| cache = clcache.Cache(tempDir) | ||
| def expectMiss(self, runCompiler): | ||
| # Build once in one directory | ||
| with cd("builddir_a"): | ||
| runCompiler[0]() | ||
| with self.cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 1) | ||
| self.assertEqual(stats.numCacheHits(), 0) | ||
|
|
||
| # Build again in a different directory, this should hit now because of CLCACHE_BASEDIR | ||
| with cd("builddir_b"): | ||
| runCompiler[1]() | ||
| with self.cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 2) | ||
| self.assertEqual(stats.numCacheHits(), 0) | ||
|
|
||
| def testBasedirRelativePaths(self): | ||
| def runCompiler(): | ||
| self._runCompiler("main.cpp") | ||
| self.expectHit([runCompiler, runCompiler]) | ||
|
|
||
| def testBasedirAbsolutePaths(self): | ||
| def runCompiler(): | ||
| self._runCompiler(os.path.join(os.getcwd(), "main.cpp")) | ||
| self.expectHit([runCompiler, runCompiler]) | ||
|
|
||
| def testBasedirIncludeArg(self): | ||
| def runCompiler(): | ||
| self._runCompiler("main.cpp", ["/I{}".format(os.getcwd())]) | ||
| self.expectHit([runCompiler, runCompiler]) | ||
|
|
||
| def testBasedirIncludeSlashes(self): | ||
| runCompiler1 = lambda: self._runCompiler("main.cpp", ["/I{}/".format(os.getcwd())]) | ||
| runCompiler2 = lambda: self._runCompiler("main.cpp", ["/I{}".format(os.getcwd())]) | ||
| self.expectHit([runCompiler1, runCompiler2]) | ||
|
|
||
| def testBasedirIncludeArgDifferentCapitalization(self): | ||
| def runCompiler(): | ||
| self._runCompiler("main.cpp", ["/I{}".format(os.getcwd().upper())]) | ||
| self.expectHit([runCompiler, runCompiler]) | ||
|
|
||
| def testBasedirDefineArg(self): | ||
| def runCompiler(): | ||
| self._runCompiler("main.cpp", ["/DRESOURCES_DIR={}".format(os.getcwd())]) | ||
| self.expectMiss([runCompiler, runCompiler]) | ||
|
|
||
| def testBasedirRelativeIncludeArg(self): | ||
| basedir = os.getcwd() | ||
|
|
||
| def runCompiler(cppFile="main.cpp"): | ||
| cmd = CLCACHE_CMD + ["/nologo", "/EHsc", "/c", "/I."] | ||
| cmd = cmd + [cppFile] | ||
| env = dict(os.environ, CLCACHE_DIR=self.clcacheDir, CLCACHE_BASEDIR=basedir) | ||
| self.assertEqual(subprocess.call(cmd, env=env), 0) | ||
|
|
||
| cmd = CLCACHE_CMD + ["/nologo", "/EHsc", "/c", "main.cpp"] | ||
|
|
||
| # Build once in one directory | ||
| with cd("builddir_a"): | ||
| env = dict(os.environ, CLCACHE_DIR=tempDir, CLCACHE_BASEDIR=os.getcwd()) | ||
| self.assertEqual(subprocess.call(cmd, env=env), 0) | ||
| with cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 1) | ||
| self.assertEqual(stats.numCacheHits(), 0) | ||
|
|
||
| shutil.rmtree("builddir_a", ignore_errors=True) | ||
|
|
||
| # Build again in a different directory, this should hit now because of CLCACHE_BASEDIR | ||
| with cd("builddir_b"): | ||
| env = dict(os.environ, CLCACHE_DIR=tempDir, CLCACHE_BASEDIR=os.getcwd()) | ||
| self.assertEqual(subprocess.call(cmd, env=env), 0) | ||
| with cache.statistics as stats: | ||
| self.assertEqual(stats.numCacheMisses(), 1) | ||
| self.assertEqual(stats.numCacheHits(), 1) | ||
| self.expectMiss([runCompiler, runCompiler]) | ||
|
|
||
|
|
||
| class TestCleanCache(unittest.TestCase): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed to make the integration test
TestBasedir.testBasedirIncludeArgpass. There the include path is generated byos.getcwd()which doesn't include the trailing separator. AsnormalizedBasediris used incollapseBasedirToPlaceholderthe version with the trailing slash would not match.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first wanted to insist on rather adjusting the test such that it appends a path separator (if needed) to the os.getcwd() return value instead of adjusting this function, but I guess if os.getcwd() does not return a trailing slash then that's a good indicator that we shouldn't do so either. So I'm okay with this. 👍