-
Notifications
You must be signed in to change notification settings - Fork 81
BUG: fix source file duplication in command line #290
Changes from 8 commits
f0227bb
40cd447
cc8a52b
ba1f7d1
3df60a6
662f7ed
bbdd257
94218fd
f69b709
df74660
0aac12b
ce9de5e
ec0fae1
2e10786
019ae2f
6c3a855
56be5e2
5de5fcc
725410e
19e7126
9079f32
7fb4480
4a7ee82
34fcb5d
9ebb89b
084b23b
275ae15
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 |
|---|---|---|
|
|
@@ -1243,15 +1243,21 @@ def parseArgumentsAndInputFiles(cmdline): | |
|
|
||
| @staticmethod | ||
| def analyze(cmdline): | ||
| # type: List[str] -> Tuple[List[Tuple[str, str]], List[str]] | ||
| options, inputFiles = CommandLineAnalyzer.parseArgumentsAndInputFiles(cmdline) | ||
| # Use an override pattern to shadow input files that have | ||
| # already been specified in the function above | ||
| inputFiles = {inputFile : '' for inputFile in inputFiles} | ||
| compl = False | ||
| if 'Tp' in options: | ||
| inputFiles += options['Tp'] | ||
| inputFiles.update({inputFile: '/Tp' for inputFile in options['Tp']}) | ||
| compl = True | ||
| if 'Tc' in options: | ||
| inputFiles += options['Tc'] | ||
| inputFiles.update({inputFile: '/Tc' for inputFile in options['Tc']}) | ||
| compl = True | ||
|
|
||
| # Now collect the inputFiles into the return format | ||
| inputFiles = list(inputFiles.items()) | ||
| if not inputFiles: | ||
| raise NoSourceFileError() | ||
|
|
||
|
|
@@ -1284,7 +1290,7 @@ def analyze(cmdline): | |
| objectFiles = [tmp] | ||
| if objectFiles is None: | ||
| # Generate from .c/.cpp filenames | ||
| objectFiles = [os.path.join(prefix, basenameWithoutExtension(f)) + '.obj' for f in inputFiles] | ||
| objectFiles = [os.path.join(prefix, basenameWithoutExtension(f)) + '.obj' for f, _ in inputFiles] | ||
|
|
||
| printTraceStatement("Compiler source files: {}".format(inputFiles)) | ||
| printTraceStatement("Compiler object file: {}".format(objectFiles)) | ||
|
|
@@ -1612,19 +1618,28 @@ def processCompileRequest(cache, compiler, args): | |
| printOutAndErr(out, err) | ||
| return exitCode | ||
|
|
||
| def formBaseCommandLine(cmdLine, sourceFiles): | ||
| # type: (List[str], List[Tuple[str, str]]) -> List[str] | ||
| setOfSources = set([sourceFile for sourceFile, _ in sourceFiles]) | ||
|
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. Minor cosmetic: you could shorten this by using a generator expression: setOfSources = set(sourceFile for sourceFile, _ in sourceFiles) |
||
| skippedArgs = ('/MP', '/Tc', '/Tp') | ||
|
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 think that |
||
| baseCmdLine = [ | ||
| arg for arg in cmdLine | ||
| if not (arg in setOfSources or arg.startswith(skippedArgs)) | ||
| ] | ||
|
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. This list comprehension works for cases like
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. Ahh, but in the second case, it would be in |
||
|
|
||
| return baseCmdLine | ||
|
|
||
| def scheduleJobs(cache, compiler, cmdLine, environment, sourceFiles, objectFiles): | ||
| baseCmdLine = [] | ||
| setOfSources = set(sourceFiles) | ||
| for arg in cmdLine: | ||
| if not (arg in setOfSources or arg.startswith("/MP")): | ||
| baseCmdLine.append(arg) | ||
| # type: (???, str, List[str], ???, List[Tuple[str, str]], List[str]) -> int | ||
| # Filter out all source files from the command line to form baseCmdLine | ||
|
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 think this comment is misleading: the
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. Will do. |
||
| baseCmdLine = formBaseCommandLine(cmdLine, sourceFiles) | ||
|
|
||
| exitCode = 0 | ||
| cleanupRequired = False | ||
| with concurrent.futures.ThreadPoolExecutor(max_workers=jobCount(cmdLine)) as executor: | ||
| jobs = [] | ||
| for srcFile, objFile in zip(sourceFiles, objectFiles): | ||
| jobCmdLine = baseCmdLine + [srcFile] | ||
| for srcFile, srcLanguage, objFile in zip(zip(*sourceFiles), objectFiles): | ||
|
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. This double- for (srcFile, srcLanguage), objFile in zip(sourceFiles, objectFiles):
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. Will do. |
||
| jobCmdLine = baseCmdLine + list(filter(None, [srcLanguage, srcFile])) | ||
|
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. Since jobCmdLine = baseCmdLine + [srcLanguage + srcFile]
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. Will do. |
||
| jobs.append(executor.submit( | ||
| processSingleSource, | ||
| compiler, jobCmdLine, srcFile, objFile, environment)) | ||
|
|
||
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.
Could it be that the failure in tests is introduced here because inputFiles.items() does not always return the items in the same order? either sorting them or using OrderedDict in line 1253 could help.
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 will try that.
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.
Even if it didn't fix the tests I think it still makes sense to keep the OrderedDict. The output of analyze() should return the same order across executions 1) to keep the tests passing and 2) to generate the same hash for the compilation task (if it is computed after, I am not sure about this). I don't know enough about this part of the code, to be checked with @frerich.
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.
The test failures have nothing to do with the order, and if I understand correctly, the hash is calculated per-file ("This causes clcache to reinvoke itself recursively for each of the source files"), making the order irrelevant. There is no reason to tell Python to preserve the order when it doesn't matter.