Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSVC tool: prevent overwriting the PCH object node builder/emitter #4444

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

MSVC tool: prevent overwriting the PCH object node builder/emitter #4444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
360fc22
Prevent overwriting the PCH object node builder/emitter.
jcbrill Nov 21, 2023
8d4f899
Set env['PCH'] to the pch file in the msvc pch_emitter if env['PCH'] …
jcbrill Dec 1, 2023
496d81d
Merge branch 'master' into jbrill-msvc-pchnode
bdbaddog Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion SCons/Tool/msvc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ def pch_emitter(target, source, env):

target = [pch, obj] # pch must be first, and obj second for the PCHCOM to work

if 'PCH' not in env or not env['PCH']:
env['PCH'] = pch

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this really needs both parts of the check I guess it's fine. The common idiom when you want to preserve any existing value (but not to also test what it is):

    env.SetDefault(PCH=pch)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at this in quite awhile.

There was a comment in the issue #2249 (#2249 (comment)) where there appeared to be a reason evaluating False was important:

I wonder if the idea is to say not to set the PCH consvar manually, and instead have the PCH builder be the one that sets it.

The suggested solution was adjusted to set env['PCH']=pch in the msvc pch_emitter when either env['PCH'] is undefined or evaluates to false.

I'll have to dig back in one of these days. Unfortunately, available time is limited in the immediate short term.

Work was suspended on this for awhile because I was convinced there was a situation/confluence of events that needed state to be saved and/or maintained which is problematic. Somewhere here I had some prototype code that looked promising.

return (target, source)


Expand All @@ -121,8 +124,16 @@ def object_emitter(target, source, env, parent_emitter):
# https://github.com/SCons/scons/issues/2505
pch=get_pch_node(env, target, source)
if pch:
if str(target[0]) != SCons.Util.splitext(str(pch))[0] + '.obj':
pch_basename = SCons.Util.splitext(str(pch))[0]
if str(target[0]) != pch_basename + '.obj':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay given the pretty constrained context, but maybe ought to use $OBJSUFFIX (though then you have to subst is before using...)

env.Depends(target, pch)
else:
src_basename, src_ext = SCons.Util.splitext(str(source[0]))
if src_basename == pch_basename and src_ext in CXXSuffixes:
# target == PCH.obj and source == PCH.cpp
# don't overwrite PCH builder for PCH.obj
target = None
source = None

return (target, source)

Expand Down
4 changes: 2 additions & 2 deletions test/MSVC/PCH-source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
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', ['Source2.cpp', 'Source1.cpp', 'foo.cpp'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment here that the order is chosen for a specific reason, so somebody doesn't resort it later.

""" % locals())

test.write('Header1.hpp', r"""
""")
Expand Down
Loading