Some fixes to sconsign #3238

mwichmann · 2018-11-10T17:00:19Z

Two locations which attempt to directly print an item tipped over on py3, so they now decode().

There seem to be cases where implicit dependencies do not have signatures, so instead of looping through the dep list and indexing into the signature list (IndexError), the two lists are now zipped, which means nothing is printed, but sconsign does not die (the zip technique is used in FS.py in the engine).

Minor PEP8 changes: spaces around operators; shorter lines; two-blanks rule around classes/functions. Also unused args changed to _ to show it was intentional.

Manpage updated slightly - the internal whichdb function explicitly looks for the .dblite suffix, so the claim that if it's not .dbm it is assumed to be dblite was not true.

sconsign still will not work on a dblite file which is not suffixed .dblite, but that is an existing problem, not a newly introduced one.

Signed-off-by: Mats Wichmann mats@linux.com

Contributor Checklist:

I have created a new test or updated the unit tests to cover the new/changed functionality.
I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
I have updated the appropriate documentation

The previous fix changed a failing line that printed something that could be bytes in Py3 to do a decode. But it turned out that value can be either str or bytes and the change failed the sconsign tests. Use a try block instead of unconditionally calling decode. Signed-off-by: Mats Wichmann <mats@linux.com>

coveralls · 2018-11-10T19:00:43Z

Coverage decreased (-1.7%) to 79.967% when pulling 1c7b5c0 on mwichmann:mdw-sconsign into b4d710b on SCons:master.

bdbaddog · 2018-11-10T19:17:04Z

src/script/sconsign.py

    try:
        bkids = entry.bsources + entry.bdepends + entry.bimplicit
        bkidsigs = entry.bsourcesigs + entry.bdependsigs + entry.bimplicitsigs
    except AttributeError:
        return None
    result = []
-    for i in range(len(bkids)):
-        result.append(nodeinfo_string(bkids[i], bkidsigs[i], "        "))
+    for bkid, bkidsig in zip(bkids, bkidsigs):


This could be list comprehension.. (which is faster than for loop.. ;)

done... though I don't think this is a performance critial place, as it's outputting a bunch of strings to stdout and won't ever do anything with results beyond that....

Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann · 2018-11-10T20:37:16Z

src/script/sconsign.py

+    @staticmethod
+    def printentries(dir, val):
+        try:
+            print('=== ' + dir + ':')


For clarity, this problem turned up on a nearly empty sconsign file, which on Python2, which did not error, displayed this way:

=== .: help: None 0 0

Using Python 3, it threw an exception:

print('=== ' + dir + ':') TypeError: must be str, not bytes

mwichmann · 2018-11-10T20:42:06Z

src/script/sconsign.py

-    result = []
-    for i in range(len(bkids)):
-        result.append(nodeinfo_string(bkids[i], bkidsigs[i], "        "))
+    result = [nodeinfo_string(bkid, bkidsig, "        ")


This problem turned up after a complex build, sconsign file is 2404476 bytes. thows an IndexError because bkidsigs is zero length, so indexing into it inside the loop failed. There are multiple instances in our build of this problem seen if we cause it not to error, but all come from the same sconscript/environment. This change causes sconsign not to error out but does not address why signatures are missing - it follows the manpage comment of "the lines are simply omitted" since in these cases the function will return an empty list aka None.

bdbaddog · 2018-11-12T16:08:23Z

doc/man/sconsign.xml

 is assumed to be a traditional
 <markup>.sconsign</markup>
 file containing the signature entries
 for a single directory.
+If neither of those is true,
+<command>sconsign</command>
+attempts to guess the format (but will


Remove will probably get it wrong..

No problem, but it's true. sconsign doesn't seem to know how to recognize the default sconsign "dblite" format in any meaningful way, it only recognizes the presence of the file extension. If not found, the effort is passed on to python's whichdb, which didn't seem to be able to guess. The sentence was intended to lead into the (existing) next sentence, which is that you can specify the format with an argument.

Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann · 2018-11-29T06:28:50Z

So what is needed to make this workable? The main issue here is "fails on bad data"... probaby claiming that should be fixed is non-controversial, but the solution may not be. Around line 286, the change does what code in the engine does - zip the two lists so "extras" are dropped. That can be seen as masking the problem, rather than fixing the generation of the problem data in the engine.

bdbaddog · 2018-11-30T03:55:34Z

I think likely the right thing to do is issue and error and exit.
Give as much info about the node in question.
If the sconsign is corrupt in any fashion, continuing to process it may not have much value.

Also perhaps a flag to keep going regardless...? (and or to just issue a warning instead of error and exit?)

Or just issue an error for each and keep going?

mwichmann · 2018-11-30T14:48:09Z

Well, in my case, it's one specific area it breaks on, so I'm happy enough for it to continue. Since the sconsign file problem doesn't seem to keep the project from building it seems it should be non-fatal to the sconsign program too. Any preferences on what the warning message should be?

mwichmann · 2018-11-30T15:31:49Z

So this is an example from our build with a message added, to prompt discussion about how it could be more informative:

=== out/linux/x86_64/debug/resource/csdk/connectivity/src/bt_le_adapter/linux:
<SCons.Script.SConscript.DefaultEnvironmentCall object at 0x7f42373d4c50>: None 0 0
SConscript: None 1533235008 6541
advertisement.c: 408a24058168ff92ab0b760a88e9ebca 1530798816 4191
Warning: missing information, 6 ids and 0 sigs

bdbaddog · 2018-11-30T16:32:25Z

Maybe add a warning count at bottom of output?
It'd be easy to miss such is a full sconsign dump?

acmorrow · 2018-12-01T15:08:25Z

Regarding warnings, is there a way to make SCons warnings fatal? If not, I'd suggest adding such a feature, maybe called --fatal-warnings or --warnings-are-errors.

mwichmann · 2018-12-01T16:17:49Z

I pursued this for a while a few months ago, when I found out calling a missing sconscript didn't fail the build. In the end, something specific to the sconscript topic went in; I still have a patch for my alternative approach sitting around, which was to add a -werror flag (syntax like -warn) which allowed warnings to be treated as errors - selectively or all of them (-werror=all). I didn't really end up pushing that approach, and I felt like it needed more thought.

Two locations which attempt to directly print an item tipped over on py3, so they now decode(). There seem to be cases where implicit dependencies do not have signatures, so instead of looping through the dep list and indexing into the signature list (IndexError), the two lists are now zipped, which means nothing is printed, but sconsign does not die (the zip technique is used in FS.py in the engine). Minor PEP8 changes: spaces around operators; shorter lines; two-blanks rule around classes/functions. Also unused args changed to _ to show it was intentional. Manpage updated slightly - the internal whichdb function explicitly looks for the .dblite suffix, so the claim that if it's not .dbm it is assumed to be dblite was not true. sconsign still will not work on a dblite file which is not suffixed .dblite, but that is an existing problem, not a newly introduced one. Signed-off-by: Mats Wichmann <mats@linux.com>

The previous fix changed a failing line that printed something that could be bytes in Py3 to do a decode. But it turned out that value can be either str or bytes and the change failed the sconsign tests. Use a try block instead of unconditionally calling decode. Signed-off-by: Mats Wichmann <mats@linux.com>

Signed-off-by: Mats Wichmann <mats@linux.com>

Rather than just silently moving on, emit warning messages if id count does not match signature count; summarize at the end if there were any warnings. Signed-off-by: Mats Wichmann <mats@linux.com>

acmorrow · 2018-12-01T16:33:06Z

@mwichmann - Sounds like a good approach though. Did it ever make it to a PR?

mwichmann · 2018-12-01T17:05:46Z

No, but I can push one up if it's interesting.

bdbaddog · 2018-12-01T20:20:21Z

Is there any reasonable way to test this?

mwichmann · 2018-12-01T22:05:32Z

test what? sconsign printing warnings? maybe there's a way to dummy up an sconsign file with the issue - there's already a test for a corrupt file. Not sure I know enough to build an artificial file. Meanwhile, until I fix our mess, we're able to reliably create a sconsign file that crashes unpatches sconsign.

bdbaddog · 2018-12-01T23:21:57Z

ok. Merging.

acmorrow · 2018-12-04T15:25:52Z

@mwichmann - I'd definitely be interested in your work to add -werror to scons. It sounds like a worthwhile feature.

mwichmann · 2018-12-04T16:04:23Z

published as #3250

bdbaddog reviewed Nov 10, 2018

View reviewed changes

mwichmann added a commit to mwichmann/scons that referenced this pull request Nov 10, 2018

For PR SCons#3238: suggestion to use a comprehension

8049230

Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann commented Nov 10, 2018

View reviewed changes

bdbaddog reviewed Nov 12, 2018

View reviewed changes

mwichmann added a commit to mwichmann/scons that referenced this pull request Nov 12, 2018

PR SCons#3238: sconsign manpage wording for unknown format.

a276d3a

Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann added 5 commits December 1, 2018 09:28

For PR SCons#3238: suggestion to use a comprehension

7cf33c6

Signed-off-by: Mats Wichmann <mats@linux.com>

PR SCons#3238: sconsign manpage wording for unknown format.

a573173

Signed-off-by: Mats Wichmann <mats@linux.com>

For sconsign, add a warning message if missing sigs

1c7b5c0

Rather than just silently moving on, emit warning messages if id count does not match signature count; summarize at the end if there were any warnings. Signed-off-by: Mats Wichmann <mats@linux.com>

mwichmann force-pushed the mdw-sconsign branch from afa1b65 to 1c7b5c0 Compare December 1, 2018 16:28

bdbaddog merged commit 55eb6f5 into SCons:master Dec 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes to sconsign #3238

Some fixes to sconsign #3238

mwichmann commented Nov 10, 2018

coveralls commented Nov 10, 2018 •

edited

Loading

bdbaddog Nov 10, 2018

mwichmann Nov 10, 2018

mwichmann Nov 10, 2018

mwichmann Nov 10, 2018

bdbaddog Nov 12, 2018

mwichmann Nov 12, 2018 •

edited

Loading

mwichmann commented Nov 29, 2018 •

edited

Loading

bdbaddog commented Nov 30, 2018

mwichmann commented Nov 30, 2018

mwichmann commented Nov 30, 2018

bdbaddog commented Nov 30, 2018

acmorrow commented Dec 1, 2018

mwichmann commented Dec 1, 2018

acmorrow commented Dec 1, 2018

mwichmann commented Dec 1, 2018

bdbaddog commented Dec 1, 2018

mwichmann commented Dec 1, 2018

bdbaddog commented Dec 1, 2018

acmorrow commented Dec 4, 2018

mwichmann commented Dec 4, 2018

Some fixes to sconsign #3238

Some fixes to sconsign #3238

Conversation

mwichmann commented Nov 10, 2018

Contributor Checklist:

coveralls commented Nov 10, 2018 • edited Loading

bdbaddog Nov 10, 2018

Choose a reason for hiding this comment

mwichmann Nov 10, 2018

Choose a reason for hiding this comment

mwichmann Nov 10, 2018

Choose a reason for hiding this comment

mwichmann Nov 10, 2018

Choose a reason for hiding this comment

bdbaddog Nov 12, 2018

Choose a reason for hiding this comment

mwichmann Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

mwichmann commented Nov 29, 2018 • edited Loading

bdbaddog commented Nov 30, 2018

mwichmann commented Nov 30, 2018

mwichmann commented Nov 30, 2018

bdbaddog commented Nov 30, 2018

acmorrow commented Dec 1, 2018

mwichmann commented Dec 1, 2018

acmorrow commented Dec 1, 2018

mwichmann commented Dec 1, 2018

bdbaddog commented Dec 1, 2018

mwichmann commented Dec 1, 2018

bdbaddog commented Dec 1, 2018

acmorrow commented Dec 4, 2018

mwichmann commented Dec 4, 2018

coveralls commented Nov 10, 2018 •

edited

Loading

mwichmann Nov 12, 2018 •

edited

Loading

mwichmann commented Nov 29, 2018 •

edited

Loading