Hi, I think this patch (which you already committed) may have some errors, can you please review my comments below? Thanks.
On Sat, Oct 13, 2018 at 3:00 AM Commit Bot <[email protected]> wrote: > From: Waldemar Kozaczuk <[email protected]> > Committer: Waldemar Kozaczuk <[email protected]> > Branch: master > > Revert automatic clean when build check and instead rebuild > boot16.o/boot32.o every time > > This patch reverts previous one to automatically clean > the build directory when build check. Instead it changes > makefile rules to rebuild boot16.o and boot32.o > everytime lzloader.elf changes. This makes the build > process slightly more robust in cases where changes > to lzloader.elf require rebuilding those 2 two objects > due to some implicit dependencies like lzkernel_base > variables. > > Signed-off-by: Waldemar Kozaczuk <[email protected]> > > --- > diff --git a/Makefile b/Makefile > --- a/Makefile > +++ b/Makefile > @@ -422,7 +422,7 @@ ifeq ($(arch),x64) > kernel_base := 0x200000 > lzkernel_base := 0x100000 > > - > +$(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? > $(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? > $(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.
