Skip to content
This repository was archived by the owner on Feb 4, 2020. It is now read-only.

Conversation

@siu
Copy link
Contributor

@siu siu commented Aug 17, 2016

clcache will throw an exception if a header file in a manifest has disappeared. How to reproduce:

  1. Rename a header file in a project
  2. Forget to update the include line in one source file
  3. Attempt to compile (direct=on, hardlink=on, cmake + ninja)

clcache will throw an exception while computing the hashes of the files included in the manifest.

Traceback (most recent call last):
  File "C:\src\clcache\clcache.py", line 1518, in <module>
    sys.exit(main())
  File "C:\src\clcache\clcache.py", line 1409, in main
    exitCode, compilerStdout, compilerStderr = processCompileRequest(cache, compiler, sys.argv)
  File "C:\src\clcache\clcache.py", line 1439, in processCompileRequest
    return processDirect(cache, objectFile, compiler, cmdLine, sourceFiles[0])
  File "C:\src\clcache\clcache.py", line 1476, in processDirect
    [expandBasedirPlaceholder(include, baseDir) for include in manifest.includeFiles])
  File "C:\src\clcache\clcache.py", line 188, in getIncludesContentHashForFiles
    listOfIncludesHashes = [getFileHash(filepath) for filepath in listOfIncludesAbsolute]
  File "C:\src\clcache\clcache.py", line 188, in <listcomp>
    listOfIncludesHashes = [getFileHash(filepath) for filepath in listOfIncludesAbsolute]
  File "C:\src\clcache\clcache.py", line 688, in getFileHash
    with open(filePath, 'rb') as inFile:
FileNotFoundError: [Errno 2] No such file or directory: 'c:\\src\\clcache\\tests\\integrationtests\\header-change\\version.h'

Note that the included test crashes with the master version of clcache but still fails with the patch because the compiler returns with error. This is expected as the code is invalid. I would like to change the test before this is merged by I am unsure about where in the tests this should be tested. What would be a better place to test this kind of behavior?

@webmaster128
Copy link
Contributor

See #179

@siu
Copy link
Contributor Author

siu commented Aug 17, 2016

My bad, I didn't see other PR. This PR can be closed when #179 is merged.

@webmaster128
Copy link
Contributor

No problem :) You cannot really know all open issues and PRs and sometimes it is good to compare different ideas to solve a problem.

Your approach is already pretty good and modulo naming and formatting, the same as #179. But there is one case not handled properly: when a header is missing and is allowed to be missing (was included by another header and got obsolete), you end up in postprocessNoManifestMiss instead of postprocessHeaderChangedMiss. This leads to source change misses in the statistics instead of header changed misses. I think/hope I did cover this in #179.

I'd be very happy if you would find the time to test #179 and check if it fits your needs.

@siu
Copy link
Contributor Author

siu commented Aug 18, 2016

Ah, now I understand the 2 cases you are trying to cover in #179. I tried #179 locally, it is working as expected.

@frerich frerich closed this in #179 Aug 22, 2016
@siu siu deleted the manifestWithDeletedHeader branch August 30, 2016 09:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants