Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bad interaction between promotion and incremental builds on OSX #460

Merged
merged 2 commits into from
Feb 1, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 29, 2018

Previously we were only using mtime but it's not precise enough on OSX.

@rgrinberg
Copy link
Member

rgrinberg commented Jan 30, 2018

CI still fails:

        diff (internal) (exit 1)
(cd _build/default && /usr/bin/diff -u test/blackbox-tests/test-cases/promote/run.t test/blackbox-tests/test-cases/promote/run.t.corrected)
--- test/blackbox-tests/test-cases/promote/run.t	2018-01-29 16:53:36.000000000 +0000
+++ test/blackbox-tests/test-cases/promote/run.t.corrected	2018-01-29 16:53:58.000000000 +0000
@@ -17,11 +17,8 @@
 
   $ echo titi > x
   $ $JBUILDER build --root . -j1 --diff-command false @blah --auto-promote 2>&1 | sed 's/.*false.*/DIFF/'
-            sh (internal) (exit 1)
-  DIFF
-  Promoting _build/default/x.gen to x.
   $ cat x
-  toto
+  titi
   $ $JBUILDER build --root . -j1 --diff-command false @blah
   $ cat x
-  toto
+  titi

With a different messages now though. Here's the other one:

        diff (internal) (exit 1)
(cd _build/default && /usr/bin/diff -u test/blackbox-tests/test-cases/promote/run.t test/blackbox-tests/test-cases/promote/run.t.corrected)
--- test/blackbox-tests/test-cases/promote/run.t	2018-01-29 16:56:33.000000000 +0000
+++ test/blackbox-tests/test-cases/promote/run.t.corrected	2018-01-29 16:56:43.000000000 +0000
@@ -12,6 +12,9 @@
   toto
 
   $ $JBUILDER build --root . -j1 --diff-command false @blah
+            sh (internal) (exit 1)
+  (cd _build/default && /bin/sh -c 'false x x.gen')
+  [1]
   $ cat x
   toto
 
@@ -23,5 +26,8 @@
   $ cat x
   toto
   $ $JBUILDER build --root . -j1 --diff-command false @blah
+            sh (internal) (exit 1)
+  (cd _build/default && /bin/sh -c 'false x x.gen')
+  [1]
   $ cat x
   toto

@ghost
Copy link
Author

ghost commented Jan 30, 2018

And obviously, I can't reproduce it on my macbook...

@ghost
Copy link
Author

ghost commented Jan 30, 2018

OSX is very frustrating:

$ echo toto > x; stat -s x |tr " " "\n" > s1; cat x > y; cat y > x; stat -s x |tr " " "\n" > s2; diff -u s1 s2
--- s1	2018-01-30 08:35:55.000000000 -0500
+++ s2	2018-01-30 08:35:55.000000000 -0500
@@ -6,7 +6,7 @@
 st_gid=1068632065
 st_rdev=0
 st_size=5
-st_atime=1517319354
+st_atime=1517319355
 st_mtime=1517319355
 st_ctime=1517319355
 st_birthtime=1517319048

@ghost ghost force-pushed the fix-promote-tests-on-osx branch from ae88da6 to 484df4a Compare January 30, 2018 14:24
@ghost
Copy link
Author

ghost commented Jan 30, 2018

Ok, I made sure that we delete the destination file before copying to force a new inode to be allocated. I also removed some old stuff about updated files, which dates from when we were using timestamps for incremental compilation.

@ghost ghost force-pushed the fix-promote-tests-on-osx branch from 484df4a to 3e7938b Compare January 30, 2018 15:01
@ghost
Copy link
Author

ghost commented Jan 30, 2018

This issue is driving me mad

@ghost ghost force-pushed the fix-promote-tests-on-osx branch from 3e7938b to bdba30b Compare January 30, 2018 17:21
@ghost
Copy link
Author

ghost commented Jan 30, 2018

Ok, so basically we just can't rely on stat on OSX. That means that we must re-hash files outside of _build on every run. I need to change the code so that we only do that on OSX. I'm also trying to see if rehashing everything is OK with a faster hash.

@ghost ghost force-pushed the fix-promote-tests-on-osx branch from bdba30b to 3cbb52a Compare January 30, 2018 22:48
@ghost ghost changed the title Use mtime + inode to detect when we need to rehash Fix bad interaction between promotion and incremental builds on OSX Jan 30, 2018
Jeremie Dimino added 2 commits February 1, 2018 08:23
This is to avoid problems with incremental compilation on OSX.

Fix #456
This dates from the time we were using timestamps for incremental
compilation.
@ghost ghost force-pushed the fix-promote-tests-on-osx branch from 04dec24 to 143145b Compare February 1, 2018 08:23
@ghost
Copy link
Author

ghost commented Feb 1, 2018

@rgrinberg, i've made the minimal necessary changes to improve the behaviour and get the tests to pass. It's ready for review.

I'll look later into the idea of systematically rehashing external files and using a faster hash implementation.

match Hashtbl.find cache fn with
| None -> ()
| Some file -> file.timestamp_checked <- false
let remove fn = Hashtbl.remove cache fn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind explaining this change? Why was setting it to false a problem before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • it was useless given that we were doing that for the targets of a rule that we always delete before running the rule anyway
  • it didn't effectively removed entries from the on-disk cache

@rgrinberg rgrinberg merged commit e61142e into master Feb 1, 2018
@rgrinberg rgrinberg deleted the fix-promote-tests-on-osx branch February 1, 2018 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant