PCH source file rebuilt every time when certain filenames are used #2249
Comments
|
Tagged this with variantdir because that (in the old form BuildDir) appears in the SConscript. |
|
Something about this sounds familiar - wasn't there some ordering issue? #2877 has one of these. |
|
The original filer is using |
|
The original filer is using I've imported the zip file and updated for python3 and BuildDir->VariantDir change. This also has the problem where the VariantDir()'s source is a parent of the variantdir..
With my updates and one more, it's not finding c.pch in static. |
|
Not exactly sure what's the issue. Here's the generated command line which get's run (after my changes.. I moved variant_dir to the SConscript call so the variantdir wasn't a child of the src dir).
All my updates are available here: https://github.com/bdbaddog/scons-bugswork/tree/main/2249 |
I'm not either. When proceeding, keep in mind I'm just trying to help. The sample files do illustrate a problem though.
There does appear to be a lexical ordering issue.
Using a recent version of scons with the build files, I had to change one line in the Original: Modified: Working thesis: Any generated pch file name that is lexically ordered before "core" fails while any generated pch file name that is lexically ordered after "core" passes. File The results from two builds with and environment dump and command-line options File
|
I guess we've each "ported" this example in different ways... Yes the sort order seems to matter, but it's not caused by using glob here (1) - the list ends up (for me at least) in alphabetical order anyway, and the key seems to be that when the source to be fed to the PCH builder is later in the source list it works, and when it's first it doesn't. I ran the example of taking a "working" setup and sorting the list in reverse, and it goes back to failing. That is: What's head-scratching there is the line which should end up building the pch comes out ordered a bit differently, though it appears to have identical contents. The two dep tree dumps @jcbrill posted have the same contents, as well, accounting for the different filename. The sort order differs, I don't know if that's because we sort the deps when we emit the tree dump, or whether SCons actually has an internal difference that matters. (1) footnote: I imagine this example uses glob.glob instead of Glob because the author is convinced of needing to do all this path fiddling, Trying to not let that get in the way of thinking about this thing... |
|
Bear in mind that I don't know what I'm talking about. The runs that pass seem to consider node What does the node In runs that fail, the taskmaster evaluates child file nodes before the "root" node. Passes: Fails: |
|
Just a WAG and likely misinformation, but it seems the difference in evaluation order of the DAG could be responsible for the difference in behavior. Maybe? |
In a case-insenstive sort, which I assume SCons thinks it needs to do because it's a Windows file system, "core" is before corF". It's probably not more than that... although this seems the wrong place to be worrying about sort order...
This is probably because SCons lets you call targets by un-suffixed names, aka |
|
We always say "the taskmaster will figure it out", but there have been more than one case where things that are essentially "side effects" which also become sources don't work out right - I think there are several closed PCH bugs about that problem, maybe it wasn't completely stamped out in the past... |
The lines to use ( The line to create ( |
|
You're absolutely right, that is the difference, it fooled my old tired eyes, because after I moved the |
|
Stupid question time. In the abstract, it appears that there is a difference when processing the DAG by the taskmaster whether or not the static library node is considered first or not. Is it possible that the the pch source file and target are "disconnected" from the DAG when evaluated prior to the static library node? That is, is it possible that the pch source/target is evaluated before a parent/child relationship is established by processing the static library node first? I don't know anything about how the DAG is constructed. "Construct and then evaluate" is different from "construct and evaluate during discovery". In the latter case, starting/root nodes might matter. |
@bdbaddog you keep saying this, but there are so many testcases that do exactly this, as well as documentation and examples, that I can't help but think it was intended to work... just from a quick search: SCons/Environment.xml:VariantDir('build', '.', duplicate=0)
SCons/Script/SConscript.xml:SConscript(dirs = 'src', variant_dir = 'build', src_dir = '.')
SCons/Script/SConscript.xml:VariantDir('build', '.')
SCons/Node/FSTests.py: fs.VariantDir('build', '.')
test/Configure/VariantDir2.py:SConscript('SConscript', variant_dir='build', src='.')
test/Configure/VariantDir.py:VariantDir('build', '.')
test/Configure/VariantDir-SConscript.py:VariantDir( 'build', '.' )
testing/framework/TestSCons.py: VariantDir(builddir, '.', duplicate=dup)
test/option/option--duplicate.py:VariantDir('build', '.', duplicate=1)
test/QT/qt3/installed.py:VariantDir('bld', '.')
test/QT/qt3/QTFLAGS.py: VariantDir('build', '.', duplicate=1)
test/VariantDir/Clean.py:VariantDir('build0', '.', duplicate=0)
test/VariantDir/Clean.py:VariantDir('build1', '.', duplicate=1)
test/VariantDir/guess-subdir.py:VariantDir(c_builddir, '.', duplicate=0)
test/VariantDir/SConscript-variant_dir.py:SConscript('SConscript', variant_dir='Build', src_dir='.', duplicate=0)
test/VariantDir/SConscript-variant_dir.py:SConscript('src/SConscript', variant_dir='build/var9', src_dir='.')
test/VariantDir/SConscript-variant_dir.py:VariantDir('build/var9', '.')
test/VariantDir/VariantDir.py:VariantDir('build', '.')
test/VariantDir/VariantDir.py:VariantDir('build', '.', duplicate=1 ) |
|
I've wasted a little more time poking at this, and I'm pretty convinced everything is consistent coming out of the sconscript phase - the PCH builder has been invoked the same way and produced the same result - and that the problem indeed happens at taskmaster time. Won't put any money on it, though :-) |
|
@mwichmann When you have time, would you please review to see if the following makes any sense. My comment about the commands in your runs is not quite true. In both runs the first compiler command is for the pch file but with different switches for pch ( The difference between the builds that fail and the builds that pass is that in the failed builds the first generated compile command is not creating the precompiled header files but instead is generated to use the precompiled header file. I tried another pair of tests. The file names were changed so they both lexically precede "core". Test 1 - [pch source file discovered first]: Test 1 - Fails: The pre-compiled header source file command-line should be creating ( Test 2 - [pch source file discovered after source file]: Test 2 - Passes: The precompiled header source file command-line is creating ( Observation: When the pch source file is discovered first, the command-line is not generated with the create ( From your run above, the first compiler calls from both runs are shown (manually adjusted the options order so they line-up visually): The only difference is the successful build creates ( |
|
Based on the generated argument order and pch switch difference for the pch source file, I'm starting to think that PCHCOM is not called/applied when the pch source file is discovered first: |
|
The problem with having the variant dir be a child of the source is that then on the second run the build dir itself is the source for Now take the above and add more than one variant where all the N variants are children of the source dir.. If it happens to work sometimes, that doesn't in any way mean it's a good idea. |
Yes, this is what I was saying, and also that there doesn't seem to be any difference coming out of the emitter in the PCH builder, so there's some other problem in the plumbing. Not sure how well the PCH logic understands the current model anyway. We have a construction variable (which this example sets): https://scons.org/doc/production/HTML/scons-man.html#cv-PCH which is per-environment, but that's not really a requirement:
|
|
Old-school print-style debugging. The builds that pass generate a command to build the PASS FAIL |
|
No, I don't get this bit either. Usually, you find header deps by scanning the source file, but that doesn't happen in the case of Microsoft's PCH, as those only come from the command line ( |
I haven't the foggiest idea (yet). I tried a handful of ways to manually add a dependency via "Depends". Everything yielded the header missing error or a cycle detected error. I may not have done it correctly but it makes me wonder if manually added a dependency causes the deferred dependency to be discovered. I'm not confident. You might have better luck trying for the runs that fail. |
|
If the I'm not saying this is a good idea and probably fails for some other reason in a real-world scenario, but it definitely illustrates that the DAG is processed/constructed differently between the two cases. When SConscript file: import glob
DEFER_PCH = True
#get all the build variables we need
Import('env', 'buildroot', 'project', 'mode', 'debugcflags', 'releasecflags')
staticlibenv = env.Clone()
builddir = buildroot + '/' + project #holds the build directory for this project
targetpath = builddir #holds the path to the executable in the build directory
#append the user's additional compile flags
#assume debugcflags and releasecflags are defined
if mode == 'debug':
staticlibenv.Append(CPPFLAGS=debugcflags)
else:
staticlibenv.Append(CPPFLAGS=releasecflags)
#get source file list
srclst = Glob('*.cpp')
staticlibenv2 = staticlibenv.Clone()
if not DEFER_PCH:
staticlibenv2['PCHSTOP'] = str(File('core/CorePCH.hpp'))
# pch.cpp is beside this SConscript
pch_nodes = staticlibenv2.PCH("a-pch.cpp")
print("PCHN: %s"%pch_nodes)
staticlibenv2['PCH'] = pch_nodes[0]
additionalcflags = ['-Yl__xge_pch_symbol']
staticlibenv2.Append(CPPFLAGS=additionalcflags)
staticlib = staticlibenv2.StaticLibrary("core", source=srclst)
if DEFER_PCH:
staticlibenv2['PCHSTOP'] = str(File('core/CorePCH.hpp'))
# pch.cpp is beside this SConscript
pch_nodes = staticlibenv2.PCH("a-pch.cpp")
print("PCHN: %s"%pch_nodes)
staticlibenv2['PCH'] = pch_nodes[0] |
|
Interesting info. Particularly odd since in the case where the order puts the non-pch-source file first alphabetically, the call to the PCH builder still takes place before the StaticLibrary builder call. I don't know how worthwhile this is to pursue further - "it's broken" on some level might be as far as it goes. I know MS pushed PCH very hard for a while, but it hasn't come up a huge amount in SCons usage, as there seem to just a few reports stretching over a very long time period. |
|
Agreed. This was far more interesting yesterday and becomes less so with each hour spent. |
|
@mwichmann I believe (most of) the behavior can be explained. The issue arises when both the pch source file is added to the source list and the env.PCH() method is called. The short answer is: don't add the pch source file to the source file list when calling env.PCH(). Observations:
There does not appear to be an inherent flaw in the taskmaster. There is however an ordering problem, and possibly a missing dependency, when the pch source file appears in the source list. For the build that fails, the node ready sequence is evaluated in a different order considering At present, I'm not sure how to what it would take to alter this behavior. SConscript: import glob
PCH_FILE_NAME = 'a-pch.cpp'
PCH_FILE = File(PCH_FILE_NAME) # remove from Glob srclist
PCH_FILE_REMOVE = True
#get all the build variables we need
Import('env', 'buildroot', 'project', 'mode', 'debugcflags', 'releasecflags')
staticlibenv = env.Clone()
builddir = buildroot + '/' + project #holds the build directory for this project
targetpath = builddir #holds the path to the executable in the build directory
#append the user's additional compile flags
#assume debugcflags and releasecflags are defined
if mode == 'debug':
staticlibenv.Append(CPPFLAGS=debugcflags)
else:
staticlibenv.Append(CPPFLAGS=releasecflags)
#get source file list
srclst = Glob('*.cpp')
if PCH_FILE_REMOVE:
srclst.remove(PCH_FILE)
staticlibenv2 = staticlibenv.Clone()
staticlibenv2['PCHSTOP'] = 'core/CorePCH.hpp'
# pch.cpp is beside this SConscript
pch_nodes = staticlibenv2.PCH(PCH_FILE_NAME)
staticlibenv2['PCH'] = pch_nodes[0]
additionalcflags = ['-Yl__xge_pch_symbol']
staticlibenv2.Append(CPPFLAGS=additionalcflags)
staticlib = staticlibenv2.StaticLibrary("core", source=srclst)Build 3 - Build 4 - Log files:
|
|
Yum. Turns out there's a specific test for this case, as a result of previous failures, #2505, which I quoted from above (that's the "a little risky" line). The test is In the test code, I can take the key line and reorder the source list, like this: #env.Program('foo', ['foo.cpp', 'Source2.cpp', 'Source1.cpp'])
env.Program('foo', ['Source1.cpp', 'Source2.cpp', 'foo.cpp']) And now the test fails! |
|
Small victories! After some reflection, I think there is a (simple) explanation for the failed build.. Let me know if this sounds reasonable or if I should put on a tin-foil hat. In the build listing above, the following modified fragment is the crux of the problem: The When the Effectively, the output target dependency on I believe this means that there is no longer a dependency between the object file Without this dependency, Plausible? |
|
Put a simpler way:
Or at least that is what I've convinced myself is happening. Too much time spent down the rabbit hole... |
|
Microsoft's PCH implementation seems quite awkward - even they admit this, sort of, for example (in a comparison of techniques): "PCH files have restrictions that make them difficult to maintain.". I can't wrap my head around how this works normally. The examples in the SCons world seem to be of the form foo.cc includes foo.h includes the headers to be precompiled. In these examples, foo.cc includes nothing else, so it makes no real sense to include that in the program being built anyway. I'm assuming reality is not always like that. Meanwhile, the way SCons includes the pch file is the msvc way: |
|
Fun fact: a second call to
If you add a second call to DefaultEnvironment(tools=[])
env = Environment(tools=['msvc', 'mslink'])
env['PCH'] = env.PCH('Source1.cpp')[0]
env['PCHSTOP'] = 'Header1.hpp'
#env.Program('foo', ['foo.cpp', 'Source2.cpp', 'Source1.cpp'])
env.Program('foo', ['Source1.cpp', 'Source2.cpp', 'foo.cpp'])
env.PCH('Source1.cpp') # <-- SECOND PCH CALLThe test passes! It works for either configuration of the source lists. Adding a second Basically, the second call after the source list is registered in StaticLibrary re-establishes the targets for the PCHFILE.obj node. This is recommended for the runs where the pch source file is in the source list. It doesn't appear to hurt in the runs where the pch source file is removed from the source list. It does have to be done twice. A single invocation after the StaticLibrary call suffers the same ordering problem depending on the file names (i.e., one run will pass one run will fail). |
|
Precompiled header standard disclaimers:
It is more likely than not that the following is not technically correct. Please correct any and all misunderstandings and flat-out untruths.
I can see where precompiled headers would be valuable when using "heavyweight" third-party c++ libraries especially those with a ton of template metaprogramming and/or template specialization. For a local project, typically the third-party libraries would not be updated frequently. For example, c++ libraries like QT, wxWidgets, OGRE, Boost, etc., .... Basically put all of the third-party includes and c/c++ system headers in a common include file and use a precompiled header file. It strikes me as more valuable for header files that are not developed in-house (e.g., windows headers, compiler system headers, third-party libraries).
In most of the examples, the pch header file is processed in its entirety so there is no reason to include the pch object file in the linker command. The pch header file does not necessarily have to be processed in it's entirety (e.g., the header stop is in the middle of the file). Given that the pch header is not processed in its entirety, maybe there is a reason for the including the pch source file and linking with the pch object file. Seems sketchy. There seems to be a variety of methods of using precompiled header files:
A modified project structure and accompanying source files are shown below. The pch source file is moved to a subdirectory so that it will not be automatically added to the source list. The pch header file (include/core/pch.h) contains straightforward includes and is included as the first line in all source files (i.e., non-header files). The pch header file will work whether or not using precompiled headers. By moving the pch source file to a subdirectory, the pch source file will not be automatically added to the source list (i.e., won't be found by Glob).
The generated pch file is a binary which is just telling the compiler which headers have already been processed so it can skip processing certain header includes and use the information from the binary. As in the microsoft examples, the normal includes are still present. In the modified layout below, all headers from the source files and source pch include file should be discoverable. In the current example, all of the included headers are discoverable from the source. The relationship between the pch header file and the generated pch binary file is the missing link (which is what I think you're getting at). This relationship can't be determined automatically by scanning the source files. The SCons PCH() method provides the mechanism to generate the pch binary file as a side-effect of compiling the pch source file and provides the necessary dependency relationship to the generated pch file. Modified Project FilesFile archive: test-2249-jcb.zip Project layout: Source Files:
Generated compile commands: Project layout (after PCH build): |
|
Cherry picking what to reply to because I just found something new:
Yes, this is the intent afaict.
According to a comment in the code (specifically, in the linker tool), the object file does have to be included, so it arranges to do so automatically: Line 139 in 3199853 I wonder if this automatic inclusion may contribute to the problem we're chasing our tails on? |
Well, that is unexpected. I don't think it applies to the examples thus far as they are building a StaticLibrary which should be using In the runs thus far;
Perhaps it should be changed and tested for a SharedLibrary? |
|
Appears to be correct for mslink: |
|
Old but useful: https://stackoverflow.com/questions/35408988/lnk2011-why-does-link-exe-require-stub-obj-to-be-linked-alongside-precompiled-h I'm starting to wonder if there could be a problem when linking with created static library in another program since the pch object file is not included when building the static library. Any thoughts? I tested the modified example above to generate a SharedLibrary and it does in fact include the pch object file when linking. |
|
@mwichmann and @bdbaddog PCH generation issue and possible code change solution explained. Description of terms used below:
The mermaid diagrams are rendered when viewing the GitHub html page. Observed BehaviorFrom above in this thread:
There is an issue when both the environment PCH method is called and the pch source file is included in the source file list that can result in build failure. Summary:
The reason why the build may succeed in some configurations and fail in other configurations when the pch source file is in the source list is described in the next section. Issue Description and IllustrationWhen both the environment PCH method is called and the pch source file is included in the source file list, the PCH.obj node has two builders/emitters assigned during construction with the second builder/emitter overwriting the original builder/emitter. This is undesirable as a taskmaster evaluation criteria has been removed and the lexical ordering of nodes affects the evaluation order during the DAG walk. The DAG walk and evaluation order is not deterministic. Builds that are successful, succeed for the wrong reason. SConscript fragment that results in State A and State B in the diagrams below (some build configurations may fail): SConscript fragment that results in State A, State B, and State C in the diagrams below (all build configurations pass): The second call to the environment PCH method resolves the issue and is presented for exposition purposes only. However, the behavior informs the proposed solution below. Node PCH.pch and PCH.obj states:
Can the dual assignment of build/emitter to PCH.obj be prevented? The possible solution below appears to solve this problem. Possible SolutionSCons.Tool.msvc.object_emitter:
Manual test results for proposed code:
With the proposed code change:
|
|
Sorry I haven't had a chance to really look at this in detail yet, but the analysis looks on point. I did put up a PR to the docs last week that suggested not including the "pch source file" as a source elsewhere, as SCons will track it correctly from there. I presume that's really not enough of a "solution"? The previously referenced issue #2505 did suggest limited confidence that the fix applied there was entirely correct, which has proven to be true, since this detail of dueling emitters was not recognized. |
|
No worries. The proposed solution above was tweaked a little and is now available for testing in PR #4444. In limited testing, it appears to "do the right thing". The PR passes for the 16 combinations of the four pairs:
The PCH-source test was modified to have "foo.cpp" at the end of the list which used to fail. It might not be the worst idea to mention than there can only be one pch node per environment in addition to the requirement that the pch source file must be c++. |
|
Now that that's in a published version, I'm happy to receive concrete suggestions for improvement (which I will undoubtedly tweak further).... https://scons.org/doc/production/HTML/scons-man.html#b-PCH |
|
@jcbrill - sorry for the delay in reviewing this and the PR. What happens if env['PCH'] is set, but the user never calls env.PCH(...) ? |
No need to apologize. I'm sorry for going missing the last few days.
Yes.
That is a really good question. Without the In the current implementation, there are different reasons for the build failure based on whether or not the pch source file is removed from the source file list:
In the suggested solution, there is an scons error regardless of whether the pch source file is in the source file list or not:
One could argue that it is more desirable to fail with an scons error rather than a c compiler error. The results of the four runs are shown below based on the following combinations: (master, branch) x (pch source file included, pch source file removed). Test results:
EDIT: ADDENDUM In the current implementation, the names of the source files matter. In the case above ( For ( In the suggested solution, the same error is produced regardless of source file naming. |
Well, in this case (rephrasing from the previous comment) - you haven't given SCons the information of how a PCH file fits into the plumbing, so it shouldn't be expected to work. If the implementation were different, it might theoretically work if the PCH file were already present in the source tree, SCons just wouldn't know how to regenerate it, but since Microsoft also requires the "PCH object file" (don't even know what to call that) to be part of the link, it can't work. I guess one could ask - should |
It might be dumb, but the no PCH() should work. the pch.cpp file would basically be a no-op right? |
|
I get more confused the more time passes. We do this in various tests, so we're setting both (as you'd expect). env['PCH'] = env.PCH('Precompiled.cpp')[0]Meanwhile, there's a routine |
This. msvc also calls get_pch_node in the object emitter. Once the pch node has been defined, the compile commands will include the pch header in the command-line (e.g., I'm not sure there is a reliable way to know if env.PCH() has been called and when it was called (before or after the env['PCH'] assignment). |
|
I wonder if the idea is to say not to set the |
|
It appears that calling env.PCH() and not defining env['PCH'] fails with a python error (at least in the suggested version). However, there is a non-trivial chance that the test configuration is completely messed up. I don't have time to investigate further tonight, but will tomorrow morning. |
The suggested solution was adjusted to set A set of 24 experiments were run using the Comments:
At present, I'm not sure that setting Experiment ResultsColumn legend for tables:
NOTE: Pass does not mean that precompiled headers are being used, simple that the library build successfully. Branch
Branch
Edits:
|
This issue was originally created at: 2008-11-01 23:58:39.
This issue was reported by:
jchristian.Scons Version: Using "vs_revamp" branch build off of the 1.1.0 release. I suspect this problem happens on the official 1.1.0 release, but I haven't checked it.
Compiler: VS2008
Defect Summary: In attempting to use precompiled headers in building a Windows project, I ran into a fairly obscure bug. Apparently, if the filename specified as the source file for the PCH generation begins with the letter 'c' or 'C', the build regenerates the PCH file every time the build is run...even if no files have changed. If I change that filename to begin with another letter, it works. Note that originally my PCH source file was called "CorePCH.cpp". In trying things, I found it could be shortened to just "C.cpp" and produce the problem.
Expected Behavior: On the second build, it should just return with an "up to date" status.
Below is my directory structure and the files needed to reproduce the issue. Note that once you get this all setup, you can make it work successfully by changing the filename
C.cppin the SConscript to justPCH.cpporXCorePCH.cpp.Let me know if you have questions.
Directory structure:
SConscript:
SConstruct:
CorePCH.cpp or C.cpp:
Hello.hpp:
Hello.cpp:
I made this a P2 because even though it is obscure, it is hella hard to track down what is going on. The workaround is to simply use another filename but this isn't apparent until many hours have been wasted. As well, I suspect the fix will be trivial and from my experience P3 and P4 defects tend to get put off practically forever.
Created an attachment (id=531)
Zip file that creates the dir structure described in the defect
Upon further investigation I have found that the filenames
A.cppthruH.cppdo not work. I have confirmed thatI.cppthruQ.cppwork. And alsoX.cpp. Other filenames were not tested because I got tired of doing it. To test a different filename, simply renameC.cppin the lib/core directory and then update the SConscript file in that same directory to use that filename.My suspicion is that the dependencies are not being generated or kept because of some defect in parsing or concatenating the filename.
Hopefully this can be fixed for 1.2
1.2 is frozen; issues that didn't make it into 1.2 are moved to 1.3.
This issue erroneously had a target milestone put on it right away, so it never got discussed and assigned to someone specific. Consequently, it's been languishing in a 1.[23] + issues@scons bucket that no one actually looks at. Change the target milestone to -unspecified- so we can discuss this at the next Bug Party and get it prioritized appropriately.
Bug party triage. Steven suggests that it might be an interaction with the heuristics for determining the drive letter, but otherwise nobody has a clue. Assign to Steven for research as a penalty for suggesting a possibility.
Next time let us assign the milestone and priority. If it doesn't pass through our process, it'll get lost as this one did.
Sorry about that, I'll do that in the future. This was my first issue submitted and I wasn't sure what to put on some of the fields :O
stevenknight => issues@scons
Bug party triage. This area has been heavily revamped. Bill to research and see if the problem still happens.
John, can you try your test case with the newest checkpoint (published yesterday) and see if it's been fixed? Add a note to this issue and let us (well, Bill, to be precise) if you still see the problem. Tks.
Bug party triage. Bump this issue to P1 so it will be reviewed at next bug party.
Zip file that creates the dir structure described in the defect
The text was updated successfully, but these errors were encountered: