Conversation
The C scanners recognised only #include and C++ #import, so a change to a resource named by #embed did not mark the embedding source out of date. Both scanners now track the resource. A quoted name is looked up relative to the including file, a bracketed name in $CPPPATH: - CScanner's regex captures the directive keyword, through a new private _ClassicCPPEmbed subclass which drops the keyword again when computing the sort key, so result ordering is unchanged. ClassicCPP itself keeps its documented three-group contract, which IDL, SWIG and RC rely on. - SCons/cpp.py, which backs the opt-in CConditionalScanner, gains the keyword in its directive table plus a do_embed handler, and honours #embed in the conditional-branch wiring of start_handling_includes and stop_handling_includes. The resource itself is deliberately not scanned: its contents are embedded as data (possibly binary) rather than preprocessed. Resolved resources are tagged on the node and filtered out by recurse_nodes(). Also fixes resolve_include() assuming the closing delimiter is the last character of the line, which turned `#embed "d.bin" limit(4)` into the filename `d.bin" limit(4`, and likewise mangled an #include carrying a trailing comment. Signed-off-by: Vassili Tchersky <vt+git@vbcy.org> Assisted-by: Claude Opus 5
|
Thanks for your attention. There are some issues already on missing support for features introduced in newer C++ standards (and C standards too, for that matter)... see for example #4517 and #4518. Aren't there feature test macros for |
Are you referring to __has_embed ? I use it as a feature test as well, like here. |
Yes. What happens if you use |
It fails, with |
| not be scanned for dependencies of its own. :func:`_scannable` drops | ||
| tagged nodes when a scanner recurses into what it found. | ||
| """ | ||
| node.attributes.embedded_resource = True |
There was a problem hiding this comment.
Tagging the resource "globally" like this seems to preclude the file in question ever being used another way in the project (say, via #include). Maybe that's fine, and nobody would ever do that, but maybe we should leave a note about that somewhere if that's an intentional limitation.
There was a problem hiding this comment.
If this nit matters, I think just dropping the embedded_resource attribute once it's seen ("consumed") in _scannable ought to do the trick. I'll let wiser folk decide if I'm just being silly.
There was a problem hiding this comment.
@vassilit -I think this is unnecessary?
If the file is xyz.bmp, there's no scanner for .bmp so it won't be scanned.
If the file is xyz.h, there's a scanner for that, and it will be scanned. (although as far as I understand the use model for #embed it wouldn't make sense to us that for a source file?
There was a problem hiding this comment.
Tagging the resource "globally" like this seems to preclude the file in question ever being used another way in the project (say, via
#include). Maybe that's fine, and nobody would ever do that, but maybe we should leave a note about that somewhere if that's an intentional limitation.
I'm not that sure that nobody would ever do that, but those who do may use explicit dependencies instead.
|
Is there a downside to having this when #embed isn't handled by the compiler? |
It would, but for some versions of the standard, this seems acceptable, e.g. C17/C18 section 6.10 paragraph 9 says: The execution of a non-directive preprocessing directive results in undefined behavior. |
That's the acceptable compiler behavior. Seems like if we knew we were dealing with a c23 or c++26 then we could enable this, otherwise disable it would provide a more accurate picture of the actual dependencies. What file suffixes are likely to be pulled in via embed? |
What I meant is if #embed is found unguarded in a source file is likely destined to be compiled with a compiler that supports it. If guarded by CPP, then yes, there is a risk of dependancies wanted only when compiled with a specific compiler and not wanted with an older one. But usually, the data is needed by the program, #embeded directly or not.
In our SCons-built project, we embed source code (.sh, .py, .go). But every file suffix is likely to be embeded, it's just data. There is another case, #embed with limit() can depend on only part of the file and we have no simple way of knowing which offsets of the file changed, so that can lead to uneeded rebuilds. |
|
One issue with your current solution, if the file is also a source, a "regular" implicit dependency, is it could be scanned recursively before it's Given the types of files you've stated could be embedded, does it really make sense to exclude the recursive dependency? Did you run into such causing an issue with your build, or were you being preemptively cautious? |
|
Just to explain the level of caution I'm taking with this PR.
|
They can be embeded but to be used as pure binary data exclusively. Recursively parsing their content would be wrong, if of course they are not included or compiled on their own elsewhere anyway. Tagging it Not merging this PR is OK, as the goal was to avoid manual
However, fixing the trailing comments on an include line is actually addressing this point. |
|
Did you run into any filetypes which led you to block the recursive scanning? Sounds like all/most of the file's you've embedded wouldn't have scanners registered and so there'd be no need to add logic to do it? |
|
To be clear, I'd like to merge all the functionality of this PR, minus the blocking recursive scanning bit. |
|
@vassilit - btw unless you're setting env['IMPLICIT_COMMAND_DEPENDENCIES']=0, then assuming the compiler is the first item in the command line, it is included in the dependencies and if that changes will cause a rebuild. That also goes for almost everything on the compile command line changing will trigger rebuilds. |
|
Did you run into a situation with your build which required precluding the embed'd file from further scanning? Or was that code just pre-emptive caution? |
It was pre-emptive caution, because #embed open the door to include absolutely everything that used to be linked as an object and/or using I haven't studied mwichmann's trick of dropping embedded_resource once seen, but it seems elegant. |
IMPLICIT_COMMAND_DEPENDENCIES doesn't do that. Unless the file(s) which are embedded have registered scanners in SCons, they will never be recursively scanned and added as implicit dependencies. Given all of this, does it still seem like theres good cause to block the recursion? |
Because the scan is multi-threaded, that solution (dropping the attribute) won't solve the issue, or solve the issue where it has no effect if it's first included by I only brought it up because you though that the compiler changing wouldn't cause a rebuild. As I said before, unless the file type being |
|
Also, the fix for #include "test.h" /* comment */ might be not necessary because it is specific to the conditionnal CPP scanner, which is commented out in CScanner() by default. |
The #embeded resource can also be an #included resource elsewhere. Be safe and recursively scan for implicit dependencies.
3 participants
#embedC23 (and c++26) preprocessor feature. (see: here for an explanation of what this is#includeBoth a small bug-fix and a new feature.
Tested manually on rmlint:
Before this PR:
#include "header.h"in code.c, modify header.h, code.c get rebuilt.#include "header.h" /* useful header */in code.c, modify header.h, code.c do not get rebuilt.#embed "py.py"in code.c, modify py.py, code.c do not get rebuilt.After:
#include "header.h"in code.c, modify header.h, code.c get rebuilt.#include "header.h" /* useful header */in code.c, modify header.h, code.c do get rebuilt.#embed "py.py"in code.c, modify py.py, code.c do get rebuilt.Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).