MD5-timestamp decider does no longer follow symlinks #3880

roblub · 2021-02-02T11:55:10Z

I reported the issue to Debian yesterday, see https://bugs.debian.org/981584, and was asked by @bdbaddog to file it here

Describe the bug

I'm attaching a simple scons-test-project.tar.gz that:

contains hello2.c file that is symbolic link to hello.c file;
and sets 'MD5-timestamp' as the decider function.

Previously (what I'm proving at the bottom of this mail) scons used
to build hello2 when hello.c was changed. In the current version of
scons only hello gets rebuilt.

Initial compilation with scons 4:

  robert@vox:/tmp/proj$ scons --debug=explain
  scons: Reading SConscript files ...
  scons: done reading SConscript files.
  scons: Building targets ...
  scons: building `hello.o' because it doesn't exist
  gcc -o hello.o -c hello.c
  scons: building `hello' because it doesn't exist
  gcc -o hello hello.o
  scons: building `hello2.o' because it doesn't exist
  gcc -o hello2.o -c hello2.c
  scons: building `hello2' because it doesn't exist
  gcc -o hello2 hello2.o
  scons: done building targets.

The following command changes both hello.c and hello2.c...

  robert@vox:/tmp/proj$ sed -i -e 's/word/WoRd/i' hello.c

... but hello2.c is not rebuilt:

  robert@vox:/tmp/proj$ scons --debug=explain
  scons: Reading SConscript files ...
  scons: done reading SConscript files.
  scons: Building targets ...
  scons: rebuilding `hello.o' because:
             `hello.c' changed
             `/usr/bin/gcc' changed
  gcc -o hello.o -c hello.c
  scons: rebuilding `hello' because:
             `hello.o' changed
             `/usr/bin/gcc' changed
  gcc -o hello hello.o
  scons: done building targets.

  robert@vox:/tmp/proj$ scons -v
  SCons by Steven Knight et al.:
  	SCons: v4.0.1.c289977f8b34786ab6c334311e232886da7e8df1, 2020-07-17 01:50:03, by bdbaddog on ProDog2020
  	SCons path: ['/usr/lib/python3/dist-packages/SCons']
  Copyright (c) 2001 - 2020 The SCons Foundation

After downgrading scons to 3.1.2+dfsg-0.1, it works as I would expect:

Initial compilation with scons 3:

  robert@vox:/tmp/proj$ scons --debug=explain
  scons: Reading SConscript files ...
  scons: done reading SConscript files.
  scons: Building targets ...
  scons: building `hello.o' because it doesn't exist
  gcc -o hello.o -c hello.c
  scons: building `hello' because it doesn't exist
  gcc -o hello hello.o
  scons: building `hello2.o' because it doesn't exist
  gcc -o hello2.o -c hello2.c
  scons: building `hello2' because it doesn't exist
  gcc -o hello2 hello2.o
  scons: done building targets.

Still both files are changed by the following command:

  robert@vox:/tmp/proj$ sed -i -e 's/word/WORd/i' hello.c

... and both are rebuilt:

  robert@vox:/tmp/proj$ scons --debug=explain
  scons: Reading SConscript files ...
  scons: done reading SConscript files.
  scons: Building targets ...
  scons: rebuilding `hello.o' because:
             `hello.c' changed
             `/usr/bin/gcc' changed
  gcc -o hello.o -c hello.c
  scons: rebuilding `hello' because:
             `hello.o' changed
             `/usr/bin/gcc' changed
  gcc -o hello hello.o
  scons: rebuilding `hello2.o' because:
             `hello2.c' changed
             `/usr/bin/gcc' changed
  gcc -o hello2.o -c hello2.c
  scons: rebuilding `hello2' because:
             `hello2.o' changed
             `/usr/bin/gcc' changed
  gcc -o hello2 hello2.o
  scons: done building targets.

  robert@vox:/tmp/proj$  scons -v
  SCons by Steven Knight et al.:
  	script: v3.1.2.bee7caf9defd6e108fc2998a2520ddb36a967691, 2019-12-17 02:07:09, by bdeegan on octodog
  	engine: v3.1.2.bee7caf9defd6e108fc2998a2520ddb36a967691, 2019-12-17 02:07:09, by bdeegan on octodog
  	engine path: ['/usr/lib/scons/SCons']
  Copyright (c) 2001 - 2019 The SCons Foundation

Required information

Link to SCons Users thread discussing your issue.
Version of SCons: 4.01, but 4.10 seems to have the same bug as well
Version of Python: 3.9
Which python distribution if applicable (python.org, cygwin, anaconda, macports, brew,etc)
How you installed SCons
What Platform are you on? (Linux/Windows and which version) Debian GNU/Linux
How to reproduce your issue? Please include a small self contained reproducer. Likely a SConstruct should do for most issues.
How you invoke scons (The command line you're using "scons --flags some_arguments")

The text was updated successfully, but these errors were encountered:

bdbaddog · 2021-02-02T16:15:02Z

Thanks for posting the issue.

jhdub23 · 2022-03-17T23:31:36Z

We just hit this bug in 4.3.0 after coming from scons-3.1.2, and it caused a lot of havoc in our company. We have disabled MD5-timestamp for now and will suffer the compile-time hit.

dmoody256 · 2022-03-18T14:34:45Z

probably this commit: 6cde83c

mwichmann · 2022-03-18T17:44:52Z

It's frustrating that the explain code also produces deceptive information (the bit about gcc changing is just wrong), but that is a different problem - unfortunately the chosen decider is not the section of code that produces the explantion, that's its own function.

Meanwhile, the commit pointed to looks dodgy to me - the getmtime function at line 734 will then return the modification time of the link itself, and thus not see that what it links to has an updated timestamp. Or am I completely missing the point?

dmoody256 · 2022-03-18T18:03:36Z

Meanwhile, the commit pointed to looks dodgy to me - the getmtime function at line 734 will then return the modification time of the link itself, and thus not see that what it links to has an updated timestamp. Or am I completely missing the point?

Yeah, probably should not have the changes to getmtime and getsize.

mwichmann · 2022-03-19T14:08:30Z

I'd say this wants some more eyeballs. #3638 was made to close #3516, problems with dangling symlinks as targets, but it seems to have left behind this problem with sources that are symlinks. As a quick test, If I remove the expanded code in the getmtime function here:

scons/SCons/Node/FS.py

Line 735 in 57369e8

def getmtime(self):

the detection of the symlinked target starts working again (for the test in this issue, hello2.o is rebuilt). There was a unit test added for #3638, which can be seen here:

scons/SCons/Node/FSTests.py

Line 3604 in 57369e8

class CleanSymlinksTestCase(_tempdirTestCase):

and that still passes after the change. I haven't had the energy to think through the whole logic here, and it would be good to work up a unit test for this case as well, so what do folks think?

jhdub23 · 2022-03-19T16:07:19Z

It's clear that the MD5 symlink behavior got broken and needs to be fixed (and a unit test added). However, is pre-clearing broken symlinks a desired behavior in the first place? My reference standard is make. Here is an example that compares behavior between make, scons-3.1.2, and scons-4.3.0. Note that "scons-3.1.2 --clean" does the "wrong" thing, in my opinion. scons should only clean the actual files that it generated (restoring the original system state as much as possible).

scons-symlink.tar.gz

bdbaddog · 2022-03-19T17:53:06Z

It's clear that the MD5 symlink behavior got broken and needs to be fixed (and a unit test added). However, is pre-clearing broken symlinks a desired behavior in the first place? My reference standard is make. Here is an example that compares behavior between make, scons-3.1.2, and scons-4.3.0. Note that "scons-3.1.2 --clean" does the "wrong" thing, in my opinion. scons should only clean the actual files that it generated (restoring the original system state as much as possible).

scons-symlink.tar.gz

Yes, (at least minimally) if SCons creates those symlinks.
SCons is not limited to the functionality of make in our scope. (by that measure md5 deciders would also not be viable.. ;)

jhdub23 · 2022-03-19T19:39:08Z

Yes, (at least minimally) if SCons creates those symlinks.
SCons is not limited to the functionality of make in our scope. (by that measure md5 deciders would also not be viable.. ;)

I absolutely agree. After further experimentation and thought, the current behavior of replacing the symlink is sane, and I withdraw my pre-clearing question. Trying to figure out a spec of when and when not to replace symlinks drove me insane :-).

bdbaddog · 2022-03-19T20:03:27Z

Yup. it's tricky. :)
Welcome to some of the guts of SCons.. ;)

mwichmann · 2022-03-20T15:39:01Z

Suggestions for a test? I'm not seeing where to put something in the unit tests, though there is a test/symlink directory in the e2e tests.

jhdub23 · 2022-03-21T01:43:37Z

It would be along the lines of @rodrigc's testcase. Make sure a file is rebuilt.

The base filesystem node class has getmtime() and getsize() functions. Those were changed in an early commit to use the lstat() method if the node represented a symbolic link. However, we actually want the information of the file the symlink points to, or we can't detect changes to the mtime or size of the underlying file, and miss rebuilds if content-timestamp is used. Added a testcase which shows the failure to rebuild from the symlinked source. Fixes SCons#3880 Signed-off-by: Mats Wichmann <mats@linux.com>

dmoody256 · 2022-10-11T07:43:17Z

probably this commit: 6cde83c

…

On Thu, Mar 17, 2022 at 6:31 PM jhdub23 ***@***.***> wrote: We just hit this bug in 4.3.0 after coming from scons-3.1.2, and it caused a lot of havoc in our company. We have disabled MD5-timestamp for now and suffer the compile-time hit. — Reply to this email directly, view it on GitHub <#3880 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOZRTBL7ZOJGEG2NHKCEOTVAO6GJANCNFSM4W6QB6AA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

mwichmann added the decider label Mar 22, 2021

mwichmann added the symlink Issues involving symbolic links label Mar 29, 2021

mwichmann mentioned this issue Mar 21, 2022

Fix content-timestamp decider for symlinks #4121

Merged

3 tasks

bdbaddog closed this as completed in #4121 May 9, 2022

gsauthof mentioned this issue Jun 12, 2022

PlatformIO not rebuilding symlinked changed files platformio/platformio-core#4316

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MD5-timestamp decider does no longer follow symlinks #3880

MD5-timestamp decider does no longer follow symlinks #3880

roblub commented Feb 2, 2021 •

edited by bdbaddog

Loading

bdbaddog commented Feb 2, 2021

jhdub23 commented Mar 17, 2022 •

edited

Loading

dmoody256 commented Mar 18, 2022

mwichmann commented Mar 18, 2022

dmoody256 commented Mar 18, 2022

mwichmann commented Mar 19, 2022

jhdub23 commented Mar 19, 2022 •

edited

Loading

bdbaddog commented Mar 19, 2022

jhdub23 commented Mar 19, 2022

bdbaddog commented Mar 19, 2022

mwichmann commented Mar 20, 2022 •

edited

Loading

jhdub23 commented Mar 21, 2022

dmoody256 commented Oct 11, 2022 via email

MD5-timestamp decider does no longer follow symlinks #3880

MD5-timestamp decider does no longer follow symlinks #3880

Comments

roblub commented Feb 2, 2021 • edited by bdbaddog Loading

bdbaddog commented Feb 2, 2021

jhdub23 commented Mar 17, 2022 • edited Loading

dmoody256 commented Mar 18, 2022

mwichmann commented Mar 18, 2022

dmoody256 commented Mar 18, 2022

mwichmann commented Mar 19, 2022

jhdub23 commented Mar 19, 2022 • edited Loading

bdbaddog commented Mar 19, 2022

jhdub23 commented Mar 19, 2022

bdbaddog commented Mar 19, 2022

mwichmann commented Mar 20, 2022 • edited Loading

jhdub23 commented Mar 21, 2022

dmoody256 commented Oct 11, 2022 via email

roblub commented Feb 2, 2021 •

edited by bdbaddog

Loading

jhdub23 commented Mar 17, 2022 •

edited

Loading

jhdub23 commented Mar 19, 2022 •

edited

Loading

mwichmann commented Mar 20, 2022 •

edited

Loading