Files with zero mtime are treated as nonexistent

[ghost]

On a CentOS 7 system:

$ tar xf ome-cmake-superbuild-0.1.0.tar.xz
tar: ome-cmake-superbuild-0.1.0/cmake/GitVersion.cmake: implausibly old time stamp 1970-01-01 01:00:00
$ ls -l ome-cmake-superbuild-0.1.0/cmake/GitVersion.cmake
-rw-r--r-- 1 rleigh lsd 236 Jan  1  1970 ome-cmake-superbuild-0.1.0/cmake/GitVersion.cmake

This source archive was packed incorrectly, and a single file has an mtime of zero (i.e. start of Unix epoch).

$ mkdir build
$ cd build
$ cmake -G Ninja ../ome-cmake-superbuild-0.1.0
$ ninja
[1/1] Re-running CMake...
[cmake re-run repeated 100 times]
ninja: error: manifest 'build.ninja' still dirty after 100 tries

The cause:

$ ninja -d explain
ninja explain: output /tmp/ome-cmake-superbuild-0.1.0/cmake/GitVersion.cmake of phony edge with no inputs doesn't exist

Clearly, the file does exist. If I touch /tmp/ome-cmake-superbuild-0.1.0/cmake/GitVersion.cmake to update the timestamp to the current time, then ninja runs perfectly. It appears to be the case that Ninja is treating files with a zero mtime as being nonexistent, which seems incorrect. Please could you consider changing that behaviour? While this is an unusual corner case--files shouldn't have a timestamp this old--it can and does happen for various reasons, and it would be nice if Ninja could work correctly under these circumstances.

Kind regards,
Roger

[ghost]

bool Cleaner::FileExists(const string& path) {
  string err;
  TimeStamp mtime = disk_interface_->Stat(path, &err);
  if (mtime == -1)
    Error("%s", err.c_str());
  return mtime > 0;  // Treat Stat() errors as "file does not exist".
        ^^^^^^^^^
}

Looks like a likely candidate. And

  void MarkMissing() {
    mtime_ = 0;
  }

Also RealDiskInterface::Stat and DiskInterface::MakeDirs. Fundamentally, you're using mtime as both a timestamp and an error code, and that's simply not possible given that all valid mtime values are also valid timestamps (even negative ones). This assumption is even stated in src/disk_interface.h:

  /// stat() a file, returning the mtime, or 0 if missing and -1 on
  /// other errors.

It's assumed in a few other places as well. I don't think your current strategy is safe, particularly when zero is often the default when unspecified in e.g. archive formats. Consider that the underlying stat(2) syscall returns ENOENT when a directory entry is not existent, independently of st_mtime. Maybe you could use a small Stat structure containing a valid flag or enum (-1 and 0 cases) plus the mtime. It could provide operator< for mtime comparison and operator(bool) or operator! for validity checking, which would provide both features unambiguously and remove the problematic behaviour observed here.

[nico]

This is a dupe of #795. Someone suggested to let an mtime of 1 instead of 0 mean "file does not exist" :-)

On #795, the latest status is that it's now easy to print a message like "ninja can't handle files with mtime of 0" instead of confusingly and silently marking them as dirty. Would that be good enough for you?

Why is the file's mtime 0 on your system?

[evmar]

mtime of 1 is a clever hack! Could also just add one to all mtimes, to
make 0 available. (We don't care about the specific value of the mtimes,
they're just a magic cookie that changes when the file changes. On Windows
they're some other random value, not a epoch-seconds-counter.)

On Tue, Apr 5, 2016 at 6:36 PM, Nico Weber notifications@github.com wrote:

This is a dupe of #795 #795.
Someone suggested to let an mtime of 1 instead of 0 mean "file does not
exist" :-)

On #795 #795, the latest
status is that it's now easy to print a message like "ninja can't handle
files with mtime of 0" instead of confusingly and silently marking them as
dirty. Would that be good enough for you?

Why is the file's mtime 0 on your system?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1120 (comment)

[ghost]

The reason for the mtime being zero in this specific instance is by using the Python tarfile module without explicitly setting the mtime on the TarInfo objects being saved into the tar file--it defaults to zero unless set by hand. This is different to the behaviour of the analogous interface in the zipfile module, which defaults to the current system time (as might be expected when creating a new file). But I've seen other tools also do similar things--zero is often the default in structures and this inevitably results in files with zero mtime being created under various conditions.

Using an mtime of 1 would indeed be a "clever hack" and would certainly work around the issue here. On the other hand, a fix such as I suggested would be not much more effort and would remove the need for any special values at all, making it robust under all circumstances. Either way would be fine with me.

Thanks for following this up,
Roger

[nico]

But the 0 mtime is never intentional, right? It seems like erroring out might be the best way forward here.

[ghost]

It's not intentional, no. But it is something which occurs relatively commonly by accident. I don't think erroring out would be appropriate; if anyone ever makes a source release with an unset timestamp, that's de facto unbuildable with ninja. Setting it to 1 would be a much nicer strategy.

[evmar]

The Windows code to convert the filesystem-representation timestamp into a
Ninja TimeStamp type is here:
https://github.com/ninja-build/ninja/blob/master/src/disk_interface.cc#L60

I'm proposing changing this code:

https://github.com/ninja-build/ninja/blob/master/src/disk_interface.cc#L193
to return st.st_mtime+1;

I believe it will solve everything at the cost of us not properly handling
files with a date of the maximal epoch (but we'll have all sorts of y2k38
problems in that area once we're anywhere near that).

On Fri, Apr 8, 2016 at 7:56 AM, Roger Leigh notifications@github.com
wrote:

It's not intentional, no. But it is something which occurs relatively
commonly by accident. I don't think erroring out would be appropriate; if
anyone ever makes a source release with an unset timestamp, that's de facto
unbuildable with ninja. Setting it to 1 would be a much nicer strategy.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#1120 (comment)

[thiblahute]

I am seeing something similar here when building inside a flatpak sandbox, basically all the files handled by ostree, meaning the "system" files have a mtime = 0 (not sure why they do that, but I think it is related to the file de-duplication sytstem). Those system files are not considered nonexistent but as "dirty", and full rebuild is triggered each time I am running ninja in that env.

When rebuilding with ninja -d explain it says that the files are dirty for example:

ninja explain: /usr/include/glib-2.0/gio/gtcpwrapperconnection.h is dirty

[alexlarsson]

For flatpak I'm going to add the patch in flatpak/flatpak#607 (comment) to our ninja build until this is figured out.

[gtristan]

I think I have a more thorough fix in #1293

[gtristan]

Just noting also, I did also file Patrick Griffis' patch used in Flatpak as pull request #1292, which is a good workaround for the problem, which we know is well tested (builds regularly pass with that applied at least).

But I did put some effort to do it thoroughly in #1293 so that the semantic is changed and any mtime is a valid one, including 0 and negative mtimes which should also be valid (not sure how the codebase stands up to negative mtimes, though). While I think #1293 is the correct fix, it is of course a bit more of an invasive change and would be good to review that (I did not end up modifying deps_log.[cc,h], I'm not sure if a change is needed there to distinguish between file existence and mtimes). I have however built a lot of GNOME modules using the #1293 branch without issue.

Whichever route is taken, it would be nice to have some fix in upstream ninja for this.

[nico]

Thanks, I merged your #1292. As said elsewhere, this looks like the superior fix to me, so I'll ignore #1293 for now if that's ok with you.

[nico]

And thanks much for the fix!

[gtristan]

Thanks, I merged your #1292. As said elsewhere, this looks like the superior fix to me, so I'll ignore #1293 for now if that's ok with you.

No worries, I'm just happy that I don't need a downstream patch :)

Maybe you can also close #795