build: find_md5 list with mod time and sorted #3445
Conversation
adschm
added
the
build/scripts/tools
|
I've raise https://bugs.openwrt.org/index.php?do=details&task_id=3360 to cover this issue |
|
Bump.... Are there any objections? |
include/depends.mk
Outdated
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| DEP_FINDPARAMS := -x "*/.svn*" -x ".*" -x "*:*" -x "*\!*" -x "* *" -x "*\\\#*" -x "*/.*_check" -x "*/.*.swp" -x "*/.pkgdir*" | |||
|
|
|||
| find_md5=find $(wildcard $(1)) -type f $(patsubst -x,-and -not -path,$(DEP_FINDPARAMS) $(2)) | mkhash md5 | |||
| find_md5=find $(wildcard $(1)) -type f $(patsubst -x,-and -not -path,$(DEP_FINDPARAMS) $(2)) -printf "%p%TY%Tm%Td%TH%TM%TS%Tz\n" | sort | mkhash md5 | |||
There was a problem hiding this comment.
Wouldn't it be easier to use e.g. %p%TF%TT%TZ?
There was a problem hiding this comment.
Hi @aparcar - Thanks for looking at this change.
I've updated the change to use the shorter form arguments and retested.
|
Thinking about the timezone, I guess it's best if dropped. If you change the TZ all find_md5 results will be changed. What do you think? |
|
Also please squash both commits into a single one |
I think this is a question of an acceptable compromise. The TZ is there to address the missed rebuild from a time change to daylight savings. I do accept that this would be a false rebuild everything scenario. If the change was less strict then failing to rebuild would lead to something undefined in the build. I do not object to removing timezone if required. |
|
I don't understand, the timezone (e.g. HST) does not include any daylight saving information, does it? |
|
Additionally, the printed information is "precise" enough (2020-12-0523:19:31.6507512920) to avoid any file changes that accidentally happen exactly 1 hour before/after (as daylight change would do). Maybe I have a misunderstanding on a lower level. |
|
Lastly, please squash both commits togehter. |
It was observed that the MD5 would not change after source files had been modified, looking deeper into the build process it was discovered that find_md5 build function makes a list of the files being built and then passes the list to a summing utility on stdin. The resultant MD5 is of the file list, not the contents of the files. The MD5 would change if the ordering of the list changed, or items were removed or deleted. The proposed fix is to add the modification time after the filename and then sort the list to prevent find returning files in a different order falsely re-triggering a rebuild. The MD5 will now change when a file is modified or files are added/removed from the list. Using 'T@' to show time in epoch for timezone independent behaviour. Signed-off-by: John Beckett <john.beckett@net2edge.com>
johnbeckettn2e
force-pushed
the
find_md5_bugfix
branch
from
3513a6c
to
81af110
Compare
|
I was assuming that the timezone would be updated to reflect summer vs winter time (GMT/BST for me) or an extra character. From looking at the man page the '@' specifier can be used to get the time since epoch including fractional seconds - this avoids the issues around a long formatted date and results in less data being added to the hash. Example time is now : The commits have now been squashed |
|
Great, I give it another test and then merge it! |
|
Merged, thank you. |
It was observed that the MD5 would not change after source files had been
modified, looking deeper into the build process it was discovered that
find_md5 build function makes a list of the files being built and then
passes the list to a summing utility on stdin. The resultant MD5 is of
the file list, not the contents of the files.
The MD5 would change if the ordering of the list changed, or items were
removed or deleted. Causing inconsistent and intermittent behaviour.
The proposed fix is to add the modification time after the filename and
then sort the list to prevent find returning files in a different order
falsely re-triggering a rebuild. The MD5 will now change when a file is
modified or files are added/removed from the list.
Signed-off-by: John Beckett john.beckett@net2edge.com