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.

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

Generator functions in Textfile subst expansion are being called with for_signature=True #4037

Closed
acmorrow opened this issue Oct 21, 2021 · 4 comments · Fixed by #4040
Closed
Labels
subst Problems with quoting, substitution
Milestone

Comments

@acmorrow
Copy link
Contributor

acmorrow commented Oct 21, 2021

Describe the bug
The following program used to write val to target, but with current HEAD, it now writes sig:

env = Environment()

def generator(source, target, env, for_signature):
    if for_signature:
        return "sig"
    return "val"

env['GENERATOR'] = generator

env.Textfile(
    target="target",
    source=[
        "@generated@",
    ],
    SUBST_DICT={
        '@generated@' : '$GENERATOR',
    },
)

Required information

  • Link to SCons Users thread discussing your issue.
    Discussed on discord with @bdbaddog and @mwichmann.

  • Version of SCons
    HEAD

  • Version of Python
    N/A

  • Which python distribution if applicable (python.org, cygwin, anaconda, macports, brew,etc)
    N/A

  • How you installed SCons
    Clone of master

  • What Platform are you on? (Linux/Windows and which version)
    Linux, but I'm pretty sure this is platform independent

  • How to reproduce your issue? Please include a small self contained reproducer. Likely a SConstruct should do for most issues.
    See the above repo. With SCons 4.2 and older the file target will be produced and contain val, but on HEAD at f6cefb2 it now contains sig. This broke one of the MongoDB custom tools.

  • How you invoke scons (The command line you're using "scons --flags some_arguments")
    You can run the above reproducer just by running scons.

@bdbaddog
Copy link
Contributor

Likely this is coming to light due to pr #4021 where env.subst() is now called with env.subst(value, raw=1) to avoid incorrectly subst'ing $$(

@mwichmann
Copy link
Collaborator

mwichmann commented Oct 22, 2021

Okay, after getting a headache going through needless recursion, it's pretty clear where this comes from: in StringSubber's expand method:

                s = s(target=lvars['TARGETS'],
                     source=lvars['SOURCES'],
                     env=self.env,
                     for_signature=(self.mode != SUBST_CMD))

The mode comes straight down from how we were called into this chain - the value of raw to the env.subst call (which is made by the Textfile action) is eventually saved as self.mode. My tenous grasp of logic suggests that last kwarg should maybe be for_signature=(self.mode == SUBST_SIG). Wonder how much stuff that would break...

@mwichmann mwichmann added the subst Problems with quoting, substitution label Oct 22, 2021
@mwichmann
Copy link
Collaborator

surprisingly, it breaks nothing in the testsuite, and fixes the case in this report.

@bdbaddog
Copy link
Contributor

Yup. Seems like the right solution to me!
That codes from 2005.. :)

@mwichmann mwichmann added this to the 4.3 milestone Oct 23, 2021
mwichmann added a commit to mwichmann/scons that referenced this issue Oct 23, 2021
If the substable element is a callable, only set for_signature
true if the mode is SUBST_SIG. Previously it was set even if
the mode was SUBST_RAW.

Fixes SCons#4037

Signed-off-by: Mats Wichmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subst Problems with quoting, substitution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants