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

Update docs on special method evaluation #4665

Merged
merged 2 commits into from
Dec 29, 2024

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Dec 16, 2024

The User Guide uses an example (in the Command chapter) of making a target name out of the source name, a special attribute, and concatenation with a new suffix. The special attribute part is not something that has been introduced. Some searching suggests it's never actually described in the User Guide, though .file, .abspath and .srcdir are used in other examples.

The attribute used, .basename, doesn't actually exist - there's a .base and a .filebase. Furthermore, the substitution suggested doesn't work. Expansion of special variables like $SOURCE into nodes is deferred - see the docstring of SCons.Subst.NLWrapper - so the internal expansion ends up trying to look up the attribute on a string, which fails with an AttributeError. The way the user guide entry is written, it was not actually evaluated: it was described as an <scons_example>, but an incomplete one, and since there was no corresponding <scons_output> the problem was not detected.

The changes fix up the example to have it use an existing attribute (.base) and do File() on the source, and add a sidebar to provide a bit of an explanation, so this isn't just "magic". A subsequent example (ex4, which I previously added) is dropped as it doesn't add enough value by itself, and the final example (formerly ex5, renamed to ex3) now includes this substitution so it's actually run by the doc machinery, and can be seen to be working correctly.

This finally fixes #2905, which was closed in 2018 as having been addressed, though the non-working example actually remained in the User Guide. The issue is also mentioned in #4660, which is not resolved by changing the docs - leaving that open, at least for the time being.

The nodes chapter was changed (only) to add named section anchors, so the sidebar could link to one section there.

This is a doc-only change.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
  • I have updated the appropriate documentation

The User Guide uses an example (in the Command chapter) of making a target
name out of the source name, a special attribute, and concatenation
with a new suffix.  The special attribute part is not something that has
been introduced. Some searching suggests it's never actually described in
the User Guide, though .file, .abspath and .srcdir are used in examples.

The attribute used, .basename, doesn't actually exist - there's a
.base and a .filebase. Furthermore, the substitution suggested doesn't
work. Expansion of special variables like $SOURCE into nodes is deferred -
see the docstring of SCons.Subst.NLWrapper - so the internal expansion
ends up trying to lookup the attribute on a string, which fails with
an AttributeError.  The way the user guide entry is written, it was
not actually evaluated: it was described as an <scons_example>, but
an incomplete one, and since there was no corresponding <scons_output>
the problem was not detected.

The changes fix up the example to have it use an existing attribute
(.base) and do File() on the source, and add a sidebar to provide a
bit of an explanation so this isn't just "magic".  A subsequent example
(ex4, which I added) is dropped as it doesn't add enough value by itself,
and the final example (formerly ex5, renamed to ex3) now includes this
substitution so it's actually run by the doc machinery, and can be
seen to be working correctly.

This finally fixes SCons#2905, which was closed in 2018 as having been addressed,
though the non-working example actually remained. The issue is
also mentioned in SCons#4660, which is not resolved by changing the docs -
leaving that open, at least for the time being.

Signed-off-by: Mats Wichmann <[email protected]>
</scons_example>
<tip><para>

The example uses a <firstterm>Node special attribute</firstterm>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only true for Command() and not true for a builder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

problem has to do with when evaluation happens, so same for all builders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, I forgot DocBook actually has a <sidebar> element. However, I like the rendering of <tip> better so leaving that, but adding a title to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I'd consider this a bug, but of course documenting it will hopefully keep people from running into that bug.
Is there anyway to indicate this in the docs? something like "currently...., but we have an issue filed to make this work so it may be resolved at some time in the future" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The word "currently" does appear in the prose; I didn't intend to say more in the "friendlier" user guide. The manpage doesn't mention this problem at all (was only trying to respond to the confusion that came from someone reading the UG), but we could add something there, maybe?

@bdbaddog bdbaddog merged commit 1589673 into SCons:master Dec 29, 2024
6 of 7 checks passed
@mwichmann mwichmann added this to the 4.9 milestone Dec 29, 2024
@mwichmann mwichmann deleted the doc/source-subst-special branch December 29, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Command example (with variable substitution) does not work
2 participants