MD5-name-timestamp decider

[bdbaddog]

This issue was originally created at: 2011-05-04 14:25:49. This issue was reported by: dvitek.

dvitek said at 2011-05-04 14:25:50

We recently ran into some issues where the MD5-timestamp decider was incorrectly deciding that a content signature was up to date because the timestamp hadn't changed. Although the timestamp had not changed, the file name had changed so that the node referred to an entirely different file. However, the decider assumed they must have the same contents due to the identical timestamps.

To be more concrete, env['ENV']['PATH'] had changed so a 64-bit targeted compiler binary was getting used instead of a 32-bit one. SCons did not rebuild any object files since the two compiler binaries had the same timestamp (they were installed at the same time) and the command line did not change.

Here is a transcript that provides and demonstrates a minimal portable test case:

$ rm -f .sconsign foo
$ cat bar
ABC
$ cat baz
def
$ touch bar baz
$ cat SConstruct
Decider('MD5-timestamp')
#Command('foo', 'bar', 'echo ABC > foo')
Command('foo', 'baz', 'echo ABC > foo')
$ scons foo --tree=all
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
echo ABC > foo
+-foo
   +-baz
scons: done building targets.
$ cat SConstruct
Decider('MD5-timestamp')
Command('foo', 'bar', 'echo ABC > foo')
#Command('foo', 'baz', 'echo ABC > foo')
$ scons foo --tree=all
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
+-foo
   +-bar
scons: done building targets.

As you can see, the second invocation of scons does not rebuild foo even though it now depends on different files with different contents.

I have attached a patch that adds a new decider and fixes what would appear to be potentially a similar issue in get_max_drift_csig (?). Let me know if this part is not necessary or is harmful. I guess I could try asserting that the file names are always the same here to double check.

You could potentially get better efficiency by combining the timestamp and path fields of FileNodeInfo into a single hash value, but I'm not sure whether it's worth it. You'd want at least a 64-bit hash if not 128. Are the path fields likely to gobble up a lot of space? I would assume you get at least two copies of every path after the sconsign file has been read into memory now, and unless the sconsign writer is clever they are most likely not shared there?

Anyway, there may be a smarter way of storing the paths to avoid space overhead both on disk and in memory, since scons might already have copies of all these strings floating around somewhere.

• Dave

dvitek said at 2011-05-04 14:26:19

Created an attachment (id=850) MD5-name-timestamp decider

dvitek attached md5namets.patch at 2011-05-04 14:26:19.

MD5-name-timestamp decider

[asqui]

We just ran in to this bug in the wild and it is pretty nasty! (Quietly returning the wrong build artefacts from cache.)

Also goes pretty strongly against the stated principle of build correctness 😕

There are a few overriding principles the SCons team tries to follow in the design and implementation.

Correctness First and foremost, by default, SCons guarantees a correct build even if it means sacrificing performance a little. We strive to guarantee the build is correct regardless of how the software being built is structured, how it may have been written, or how unusual the tools are that build it.

https://scons.org/doc/production/HTML/scons-user/pr01.html#id1278

[bdbaddog]

We just ran in to this bug in the wild and it is pretty nasty! (Quietly returning the wrong build artefacts from cache.)

Also goes pretty strongly against the stated principle of build correctness 😕

There are a few overriding principles the SCons team tries to follow in the design and implementation. Correctness First and foremost, by default, SCons guarantees a correct build even if it means sacrificing performance a little. We strive to guarantee the build is correct regardless of how the software being built is structured, how it may have been written, or how unusual the tools are that build it.

https://scons.org/doc/production/HTML/scons-user/pr01.html#id1278

@asqui Can you provide more information? Which version of SCons? Which version of Python? Which platform? A small reproducer is always helpful as well. Which decider are you using?

[asqui]

Thanks for the quick response!

Here are the details you asked for: Which version of SCons? 4.5.2 Which version of Python? 3.12.2 Which platform? RHEL 8.6 A small reproducer is always helpful as well. I think the reproducer in the OP is representative of our scenario Which decider are you using? MD5-timestamp as per the original bug report

[bdbaddog]

@asqui - Please try the latest SCons verison 4.8.1 and let us know if that resolves it or not.

[asqui]

We can try... but is there any reason to believe the problem is resolved given this issue is still open?

[mwichmann]

If I try the example above, I don't get, using current git master, the same behavior as reported (it does rebuild on the second call), plus you mentioned cache, which the original does not. Could be I'm doing something funky.

But this scenario is always going to be a little quirky, because the cached file has a hash generated from the command line, which is also its name; if that were recomputed (not the same as the content hash), you'd notice a change in the command line but the entire purpose of the content-timestamp decider is to try to short-circuit computing hashes, which means it uses the existing node info fetched from the sconsign. I'm not sure what else can really happen.

The approach of the attached patch is to implement an additional decider which uses additional information in its decision. You actually don't need to change SCons itself to add a new decider. Unfortunately, the approach proposed needs an additional field to be added to the information structure, which would mean bumping the version of the FileNodeInfo class, which means either a compatibility break between versions or needing to carry transitional code... possibly why nothing happened with the proposed patch. Only speculating on that, wasn't around the project in 2011. @bdbaddog, is your memory that good? Any thoughts?

[bdbaddog]

We can try... but is there any reason to believe the problem is resolved given this issue is still open?

@asqui - There have been non-trivial changes since 4.5.2, and we'd never make a patch/fix for 4.5.2, so trying the latest released version is always a good idea when submitting an issue to any project.

@mwichmann - honestly don't remember, but likely as you said it would require a compatibility break, and we didn't have many (any?) other users reporting the issue, would be my somewhat educated guess.

[mwichmann]

As an aside - the change to selectable hash functions rather than hard-coding just md5 means the patch would need a bit of updating if someone wanted to try to resurrect it.

[cslamber]

@asqui - Please try the latest SCons verison 4.8.1 and let us know if that resolves it or not.

Confirmed that this issue is reproducible on 4.8.1.

[mwichmann]

@asqui - Please try the latest SCons verison 4.8.1 and let us know if that resolves it or not.

Confirmed that this issue is reproducible on 4.8.1.

Thanks for the update. Wonder why I couldn't make it fail

[mwichmann]

Okay, here's a capture of running through the original script again on my dev box (Fedora 40, Python 3.12, SCons master (which is 4.8.1+whatever changes have happened since). I've added an extra option to ask scons to show its decision-making. Notice it does detect the dependency change and that's what is causing the rebuild. I don't know why this is different from what you're seeing.

$ rm .sconsign.dblite foo
$ cat baz
def
$ cat bar
ABC
$ touch bar baz
$ cat SConstruct
Decider('MD5-timestamp')
#Command('foo', 'bar', 'echo ABC > foo')
Command('foo', 'baz', 'echo ABC > foo')
$ scons foo --tree=all
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
echo ABC > foo
+-foo
  +-baz
  +-/bin/echo
scons: done building targets.
$ vi SConstruct
$ cat SConstruct
Decider('MD5-timestamp')
Command('foo', 'bar', 'echo ABC > foo')
#Command('foo', 'baz', 'echo ABC > foo')
$ scons foo --tree=all --debug=explain
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
scons: rebuilding `foo' because:
           `baz' is no longer a dependency
           `bar' is a new dependency
           `/bin/echo' changed
echo ABC > foo
+-foo
  +-bar
  +-/bin/echo
scons: done building targets.
$

[bdbaddog]

@cslamber - are you using the example initially posted with this issue, or your internal SCons logic which is similar?

Is it possible whatever command line this applies to has wrapped the source file in $( $) 's?

[cslamber]

I'm using my own SCons logic with reproducer:

# SConstruct
env = Environment()
CacheDir('cache')

def trivial(target, source, env):
    with open(target[0].get_abspath(), 'w') as f:
        f.write('trivial\n')

def writename(target, source, env):
    with open(target[0].get_abspath(), 'w') as f:
        f.write(source[0].srcnode().get_internal_path())

A = File('SOURCE_A.txt')

env.Command(target=A, action=Action(trivial), source=[])
env.Command(target='NAME_A.txt', source=A, action=Action(writename))

In a directory with only this file, I run scons, then modify the file to change "SOURCE_A" to any other string, and rerun. This does not cause NAME_A.txt to rebuild. Deleting it and rerunning scons successfully hits the cache and makes NAME_A.txt still contain "SOURCE_A".

[mwichmann]

My rebuild of the new example after modifying looks like this:

$ scons --tree=all --debug=explain
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
scons: rebuilding `NAME_A.txt' because `SOURCE_A.txt' changed
writename(["NAME_A.txt"], ["SOURCE_A.txt"])
+-.
  +-NAME_A.txt
  | +-SOURCE_A.txt
  +-SConstruct
  +-SOURCE_A.txt
scons: done building targets.

[cslamber]

By "modify SOURCE_A" I mean the string in SConstruct, not the contents of the file. e.g. if I change it to A = File("SOURCE_B.txt") then it builds a SOURCE_B.txt but leaves NAME_A.txt untouched.

I originally hit this by having an action that was functionally tar-ing a bunch of files, I renamed a directory and the old file kept getting pulled from cache.

[mwichmann]

Okay, I can see the behavior you're seeing now. Looks to me because the content signatures are the same and the build signatures are the same, that there's indeed "nothing to do":

SOURCE_A.txt: {'csig': '070a33ee16c6ea794772fea49cc63f57', 'timestamp': 1737730611, 'size': 8, '_version_id': 2}
        afaed5df78327e521468b523d579ea93 [trivial(target, source, env)]
SOURCE_Z.txt: {'csig': '070a33ee16c6ea794772fea49cc63f57', 'timestamp': 1737730640, 'size': 8, '_version_id': 2}
        afaed5df78327e521468b523d579ea93 [trivial(target, source, env)]

From that dump, it looks like when the "build command" is a function call, it doesn't take the values of the arguments into consideration when computing the signature. To a human reader, those do differ, but the display string comes from a different algorithm.

trivial(["SOURCE_A.txt"], [])
trivial(["SOURCE_Z.txt"], [])

[mwichmann]

@cslamber do you mind if I move this to a different issue? It doesn't seem to be caused by using a timestamp-then-hash decider, which the original problem in this issue definitely is.

[bdbaddog]

@cslamber does your actual code have a command with no sources as the file in question? And do none of the actions involved include the source file names on the command line?

[cslamber]

No, that trivial is just for demonstration to create the file. The only necessary command is the second (writename). The actual code is a function that tars a set of files by their internal paths, so the command line is just scons.

[bdbaddog]

No, that trivial is just for demonstration to create the file. The only necessary command is the second (writename). The actual code is a function that tars a set of files by their internal paths, so the command line is just scons.

I wasn't asking about the command you type to run scons (sorry for the confusion), but rather the command line used by SCons to build your target.

@cslamber - your sample reproducer will lead us down the wrong path it sounds like. Can you create a more realistic reproducer and just create a github repo and share that?

[mwichmann]

Meanwhile... in this case, if the timstamp-content decider is used, this particular example does detect a need to rebuild:

scons: building `SOURCE_Z.txt' because it doesn't exist
CacheRetrieve(SOURCE_Z.txt):  1d73c40c0445892329f62492f64c1bd1 not in cache
trivial(["SOURCE_Z.txt"], [])
CachePush(SOURCE_Z.txt):  pushing to 1d73c40c0445892329f62492f64c1bd1
scons: rebuilding `NAME_A.txt' because:
           `SOURCE_A.txt' is no longer a dependency
           `SOURCE_Z.txt' is a new dependency
Retrieved `NAME_A.txt' from cache

[cslamber]

Is this ok?

import subprocess, os, shutil
os.makedirs('src/a', exist_ok=True)
with open('src/a/file.txt', 'w') as f:
    f.write('asdf\n')
with open('SConstruct', 'w') as f:
    f.write('''import os
env = Environment()
CacheDir('cache')
def writename(target, source, env):
    with open(target[0].get_abspath(), 'w') as f:
        f.write(source[0].srcnode().get_internal_path())

s = Dir('src/' + os.listdir('src')[0])
env.Command(target='name_a.txt', source=s.File('file.txt'), action=writename)
''')
subprocess.check_output('scons')
shutil.move('src/a', 'src/b')
subprocess.check_output('scons')
# os.remove('name_a.txt')
# subprocess.check_output('scons')