On Sun, Oct 14, 2018 at 6:22 PM Waldek Kozaczuk <[email protected]> wrote:
> > > On Sun, Oct 14, 2018 at 10:43 Nadav Har'El <[email protected]> wrote: > >> >> - >>> +$(out)/arch/x64/boot16.o: $(out)/lzloader.elf >>> >> >> I think this dependency is wrong, and can be removed - boot16.S doesn't >> use lzloader.elf >> so boot16.o doesn't need to be rebuilt if lzloader.elf changed... Unless >> I'm missing something? >> > This is more of a catch-all for the indirect dependencies between > boot16.so which loads lzloader.elf in runtime. One such dependency is > lzlernel_base which changed as part of almost-in-place kernel decompression > and caused build process get stuck on Jenkins VMs. Plus rebuilding > boot16.so is very cheap. Is there a better way like saving value of > lzkernel_base when changes to a file and having boot16.s depend of this > file? > Yes, the best way would be to do what we do for the OSv version, which we put in version.h and make sure to not modify this file if it hasn't changed from the last compilation, to not cause unnecessary compilations just because we make a new version.h file every time. See "perhaps-modify-version-h" in Makefile. What you did is just to "almost always" recompile boot16.o... Most changes to anything will cause lzloader.elf to be rebuilt, and so also boot16.o. But it's not clear to me that this "almost always" is what we should do. In any case, if you look deeper down at what caused this problem, it was a change to the Makefile. We can never really solve all these problems which stem from the *Makefile* itself being changed. If no source file changed and just the Makefile changed, we may need to recompile everything because we have no idea how the Makefile was changed... Changing lzkernel_base is just one possible change that perhaps happened this time. Another possible change is a change to one of the compilation options, in which case we need to recompile everything, but Makefile doesn't currently know to do that. In https://savannah.gnu.org/bugs/?51495 I discussed this as a possible improvement to "make", which the "ninja" build tool (which we use in the Seastar and Scylla projects) already supports. Someone suggested a hack to approximate this, but I'm sure better hacks can be thought of without modifying make, if we ever want to do this. But I think we have no choice but to have a way to do a "make clean" on Jenkins - either every time (like you did in your original patch inside "make check") or have a way to do it manually. Maybe we should do it every time, in the Jenkins script - always run "scripts/build clean" before "make check". What do you think? It will slow down Jenkins, but we don't make many commits anyway, nowadays. > >> >> >>> $(out)/boot.bin: arch/x64/boot16.ld $(out)/arch/x64/boot16.o >>> $(call quiet, $(LD) -o $@ -T $^, LD $@) >>> >>> @@ -435,11 +435,12 @@ $(out)/loader.img: $(out)/boot.bin >>> $(out)/lzloader.elf >>> $(call quiet, scripts/imgedit.py setsize "-f raw $@" >>> $(image-size), >>> IMGEDIT $@) >>> $(call quiet, scripts/imgedit.py setargs "-f raw $@" $(cmdline), >>> IMGEDIT >>> $@) >>> >>> +$(out)/arch/x64/boot32.o: $(out)/lzloader.elf >>> >> >> boot32.S does.incbin "loader-stripped.elf", so it needs to depend on >> loader.stripped.elf. >> This is what the old Makefile did. >> I doesn't need to depend on lzloader.elf. >> Why does it? Am I missing something? >> > Indeed my mistake. > >> >> >> >> >>> $(out)/loader.bin: $(out)/arch/x64/boot32.o arch/x64/loader32.ld >>> $(call quiet, $(LD) -nostartfiles -static -nodefaultlibs -o $@ \ >>> $(filter-out %.bin, $(^:%.ld=-T %.ld)), LD $@) >>> >>> -$(out)/arch/x64/boot32.o: $(out)/loader-stripped.elf >>> +$(out)/arch/x64/boot32.o: $(out)/lzloader.elf >>> >> >> You added this line *twice* in this patch - I don't think it's correct >> even once, but twice is even more unnecessary :-) >> >> >>> $(out)/arch/x64/boot32.o: ASFLAGS += -I$(out) >>> >>> $(out)/fastlz/fastlz.o: >>> diff --git a/scripts/build b/scripts/build >>> --- a/scripts/build >>> +++ b/scripts/build >>> @@ -26,9 +26,7 @@ stage1_args="stage1" >>> for i >>> do >>> case $i in >>> - image=*|modules=*|fs=*|usrskel=*) ;; >>> - check) >>> - make clean ;; >>> + image=*|modules=*|fs=*|usrskel=*|check) ;; >>> clean) >>> stage1_args=clean ;; >>> *) # yuck... Is there a prettier way to append to array? >>> @@ -53,8 +51,6 @@ do >>> set -x >>> OSV_BASE=`pwd` ./scripts/module.py clean -q >>> exit;; >>> - check) >>> - OSV_BASE=`pwd` ./scripts/module.py clean -q ;; >>> esac >>> done >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "OSv Development" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
