| Message ID | 1323107263-1870-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Dec 5, 2011 at 12:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > Minor tweaks to the $(KERNEL) target: > * use $(MAKE) so -n &c work better > * depend on the uImage which we use, not the zImage which we don't I don't appreciate this. Simple 'make' in the kernel dir produces a zImage, which is what I try to pick up - I think hack, make, hack, make, etc. is a pretty standard work flow and uImage needs zImage to be written as well, so 'make uImage' will also produce a zImage... > * use automatic variables to avoid repeating the path to uImage > * drop unnecessary separate cd > * fix stray direct use of "../linux-kvm-arm" > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Makefile | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index 6dc9912..3d74ac0 100644 > --- a/Makefile > +++ b/Makefile > @@ -35,9 +35,9 @@ all: $(IMAGE) > clean: > rm -f $(IMAGE) boot.o model.lds monitor.o uImage > > -$(KERNEL): ../linux-kvm-arm/arch/arm/boot/zImage > - cd $(KERNEL_SRC); make -j4 uImage > - cp $(KERNEL_SRC)/arch/arm/boot/uImage $(KERNEL) > +$(KERNEL): $(KERNEL_SRC)/arch/arm/boot/uImage > + $(MAKE) -C $(KERNEL_SRC) -j4 uImage > + cp $< $@ > > $(IMAGE): boot.o monitor.o model.lds $(KERNEL) $(FILESYSTEM) Makefile > $(LD) -o $@ --script=model.lds > -- > 1.7.5.4 >
On 7 December 2011 05:48, Christoffer Dall <cdall@cs.columbia.edu> wrote: > On Mon, Dec 5, 2011 at 12:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Minor tweaks to the $(KERNEL) target: >> * use $(MAKE) so -n &c work better >> * depend on the uImage which we use, not the zImage which we don't > > I don't appreciate this. Simple 'make' in the kernel dir produces a > zImage, which is what I try to pick up - I think hack, make, hack, > make, etc. is a pretty standard work flow and uImage needs zImage to > be written as well, so 'make uImage' will also produce a zImage... But this boot-wrapper makefile doesn't (and didn't) use the zImage, it uses the uImage, and that's what it asks the kernel makefile to create. I don't understand why you'd want to depend on something you're not using -- surely that can only lead to problems with not always rebuilding when necessary? -- PMM
On Wed, Dec 7, 2011 at 3:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2011 05:48, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> On Mon, Dec 5, 2011 at 12:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Minor tweaks to the $(KERNEL) target: >>> * use $(MAKE) so -n &c work better >>> * depend on the uImage which we use, not the zImage which we don't >> >> I don't appreciate this. Simple 'make' in the kernel dir produces a >> zImage, which is what I try to pick up - I think hack, make, hack, >> make, etc. is a pretty standard work flow and uImage needs zImage to >> be written as well, so 'make uImage' will also produce a zImage... > > But this boot-wrapper makefile doesn't (and didn't) use the zImage, > it uses the uImage, and that's what it asks the kernel makefile > to create. I don't understand why you'd want to depend on something > you're not using -- surely that can only lead to problems with > not always rebuilding when necessary? > I don't think it can lead to any problems. The dependency graph is this: zImage <- uImage <- boot-wrapper so if uImage has changed, so has zImage. Or am I missing something? Anyway, it's not a big issue for me, I am just trying to avoid someone building their kernel using plain 'make' and not understanding why the boot-wrapper doen't reflect those changes.
On 7 December 2011 14:39, Christoffer Dall <cdall@cs.columbia.edu> wrote: > The dependency graph is this: > > zImage <- uImage <- boot-wrapper > > so if uImage has changed, so has zImage. Or am I missing something? > > Anyway, it's not a big issue for me, I am just trying to avoid someone > building their kernel using plain 'make' and not understanding why the > boot-wrapper doen't reflect those changes. Ah, I understand the problem now. In fact patch 6 fixes this, because the 'force' dependency on the $(KERNEL_SRC)/arch/arm/boot/uImage target means we will always run the rule for that target, ie we will always invoke the kernel makefile and ask it to make sure the uImage is up to date. (I just tested this and it does do the right thing.) I hadn't spotted this wrinkle, or I'd have combined the two patches somehow instead. -- PMM
On Wed, Dec 7, 2011 at 10:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2011 14:39, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> The dependency graph is this: >> >> zImage <- uImage <- boot-wrapper >> >> so if uImage has changed, so has zImage. Or am I missing something? >> >> Anyway, it's not a big issue for me, I am just trying to avoid someone >> building their kernel using plain 'make' and not understanding why the >> boot-wrapper doen't reflect those changes. > > Ah, I understand the problem now. > > In fact patch 6 fixes this, because the 'force' dependency on the > $(KERNEL_SRC)/arch/arm/boot/uImage target means we will always run > the rule for that target, ie we will always invoke the kernel makefile > and ask it to make sure the uImage is up to date. (I just tested this > and it does do the right thing.) > > I hadn't spotted this wrinkle, or I'd have combined the two patches > somehow instead. > And I missed the other one. OK, we're in agreement.
On 7 December 2011 15:04, Christoffer Dall <cdall@cs.columbia.edu> wrote: > On Wed, Dec 7, 2011 at 10:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> In fact patch 6 fixes this, because the 'force' dependency on the >> $(KERNEL_SRC)/arch/arm/boot/uImage target means we will always run >> the rule for that target, ie we will always invoke the kernel makefile >> and ask it to make sure the uImage is up to date. (I just tested this >> and it does do the right thing.) >> >> I hadn't spotted this wrinkle, or I'd have combined the two patches >> somehow instead. > And I missed the other one. OK, we're in agreement. Do you want me to redo the patchset to combine the two patches? -- PMM
On Wed, Dec 7, 2011 at 10:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2011 15:04, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> On Wed, Dec 7, 2011 at 10:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> In fact patch 6 fixes this, because the 'force' dependency on the >>> $(KERNEL_SRC)/arch/arm/boot/uImage target means we will always run >>> the rule for that target, ie we will always invoke the kernel makefile >>> and ask it to make sure the uImage is up to date. (I just tested this >>> and it does do the right thing.) >>> >>> I hadn't spotted this wrinkle, or I'd have combined the two patches >>> somehow instead. > >> And I missed the other one. OK, we're in agreement. > > Do you want me to redo the patchset to combine the two patches? > I don't care too much either way, as long as it's included. Since you re-based on Marc's tree, I assumed he would merge how he wanted to? I actually added a small change in my repo (https://github.com/virtualopensystems/boot-wrapper/commits/master) to discover the host IP for nfs boots at compile time. Take a look and tell me if it's something you think we should merge as well. -Christoffer
On 7 December 2011 15:11, Christoffer Dall <cdall@cs.columbia.edu> wrote: > I actually added a small change in my repo > (https://github.com/virtualopensystems/boot-wrapper/commits/master) to > discover the host IP for nfs boots at compile time. Take a look and > tell me if it's something you think we should merge as well. I think the right way to do that is that we should put in the "-include config" thing, and then you can write a config file which sets KCMD to something with a make $(shell) command in it. -- PMM
-Christoffer On Dec 7, 2011, at 10:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2011 15:11, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> I actually added a small change in my repo >> (https://github.com/virtualopensystems/boot-wrapper/commits/master) to >> discover the host IP for nfs boots at compile time. Take a look and >> tell me if it's something you think we should merge as well. > > I think the right way to do that is that we should put in the > "-include config" thing, and then you can write a config file > which sets KCMD to something with a make $(shell) command in it. > > -- PMM > _______________________________________________ > Android-virt mailing list > Android-virt@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
On Dec 7, 2011, at 10:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 December 2011 15:11, Christoffer Dall <cdall@cs.columbia.edu> wrote: >> I actually added a small change in my repo >> (https://github.com/virtualopensystems/boot-wrapper/commits/master) to >> discover the host IP for nfs boots at compile time. Take a look and >> tell me if it's something you think we should merge as well. > > I think the right way to do that is that we should put in the > "-include config" thing, and then you can write a config file > which sets KCMD to something with a make $(shell) command in it. > Ok
I'm not sure I agree with this particular change; take for example some potential contributor pulling from the git tree, and blissfully building the bootwrapper, unaware that his host IP address is included automagically. This could result in confusion when that same user changes the host IP address and ends up being unable to boot the system without knowing why. So I'd rather have the user include his IP address explicitly. Compared to all the pains one has to go to setup NFS in the first place, editing a Makefile or config file is trivial anyway. Also, this assumes that the NFS server used is the same as the host, which might not be the case. For example I could be syncing my dev environment between my desktop and laptop, but I might keep the NFS filesystem only on the desktop. Etc. Anyway in any case, I didn't notice that small change before, so I'll keep it in mind to update the guide accordingly. Best regards, Antonios On 12/07/2011 04:11 PM, Christoffer Dall wrote: > On Wed, Dec 7, 2011 at 10:07 AM, Peter Maydell<peter.maydell@linaro.org> wrote: >> On 7 December 2011 15:04, Christoffer Dall<cdall@cs.columbia.edu> wrote: >>> On Wed, Dec 7, 2011 at 10:02 AM, Peter Maydell<peter.maydell@linaro.org> wrote: >>>> In fact patch 6 fixes this, because the 'force' dependency on the >>>> $(KERNEL_SRC)/arch/arm/boot/uImage target means we will always run >>>> the rule for that target, ie we will always invoke the kernel makefile >>>> and ask it to make sure the uImage is up to date. (I just tested this >>>> and it does do the right thing.) >>>> >>>> I hadn't spotted this wrinkle, or I'd have combined the two patches >>>> somehow instead. >>> And I missed the other one. OK, we're in agreement. >> Do you want me to redo the patchset to combine the two patches? >> > I don't care too much either way, as long as it's included. > > Since you re-based on Marc's tree, I assumed he would merge how he wanted to? > > I actually added a small change in my repo > (https://github.com/virtualopensystems/boot-wrapper/commits/master) to > discover the host IP for nfs boots at compile time. Take a look and > tell me if it's something you think we should merge as well. > > -Christoffer > _______________________________________________ > Android-virt mailing list > Android-virt@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
On Wed, Dec 7, 2011 at 2:01 PM, Antonios Motakis <a.motakis@virtualopensystems.com> wrote: > I'm not sure I agree with this particular change; take for example some > potential contributor pulling from the git tree, and blissfully building > the bootwrapper, unaware that his host IP address is included > automagically. This could result in confusion when that same user > changes the host IP address and ends up being unable to boot the system > without knowing why. I'm not too concerned about this and if the IP address changes, the script uses the new IP address. > > So I'd rather have the user include his IP address explicitly. Compared > to all the pains one has to go to setup NFS in the first place, editing > a Makefile or config file is trivial anyway. > I think the idea is to let uses put the script in their config file if they want or put a hard-coded address in their config file. There can be a config.sample that has everything commented out and users have to edit things in explicitly. My point of view is to make things as easy as possible for the common case and keeping configuration changes away from git commit changes.
On 12/07/2011 08:06 PM, Christoffer Dall wrote: > I'm not too concerned about this and if the IP address changes, the > script uses the new IP address. Yes, but a user in this situation would be unaware that he has to recompile the boot-wrapper just because he moved his laptop for example. > I think the idea is to let uses put the script in their config file if > they want or put a hard-coded address in their config file. There can > be a config.sample that has everything commented out and users have to > edit things in explicitly. Yes, I think this would be the best approach. > My point of view is to make things as easy as possible for the common > case and keeping configuration changes away from git commit changes. I fully agree, especially with the second part.
On Wed, Dec 7, 2011 at 2:19 PM, Antonios Motakis <a.motakis@virtualopensystems.com> wrote: > On 12/07/2011 08:06 PM, Christoffer Dall wrote: >> >> I'm not too concerned about this and if the IP address changes, the >> script uses the new IP address. > > Yes, but a user in this situation would be unaware that he has to recompile > the boot-wrapper just because he moved his laptop for example. In which case it would not work anyhow - the boot string is embedded inside the binary and a re-compile cannot be avoided. I think we are not talking about casual users using boot-wrapper in any case. It's just a question of making it easy for developers to run the whole setup. Personally, I just want to go to the boot-wrapper directory, do 'make' and be on my merry way. Not copy output from ifconfig manually and stuff if I can avoid it.
On 7 December 2011 19:52, Christoffer Dall <cdall@cs.columbia.edu> wrote: > I think we are not talking about casual users using boot-wrapper in > any case. It's just a question of making it easy for developers to run > the whole setup. Yes, exactly. We want convenient, not gold-plated :-) > Personally, I just want to go to the boot-wrapper directory, do 'make' > and be on my merry way. Not copy output from ifconfig manually and > stuff if I can avoid it. If your NFS server is the local machine you still need to update /etc/exports, of course :-( -- PMM
> > If your NFS server is the local machine you still need to update > /etc/exports, of course :-( > yes, but let's not try to fix that this time around ;)
On 12/07/2011 08:52 PM, Christoffer Dall wrote: > > In which case it would not work anyhow - the boot string is embedded > inside the binary and a re-compile cannot be avoided. Yes I know, my point is that the user is not exposed to the fact that his IP address is being used. Before, the developer did know that his IP is used in the binary, because he put it in there himself, so he knows to do the right thing. > I think we are not talking about casual users using boot-wrapper in > any case. It's just a question of making it easy for developers to run > the whole setup. Agreed, my point is that if it is being done by a script that is called without the user being aware of it, then even a developer might miss the fact that the output is specific to the host IP address. Setting it in a config file or Makefile makes it pretty clear, even if by default we but with backticks a command returning the current IP; however the way it is now, where it is being called transparently from Make, it can be confusing, because even developers are usually not used to this kind of sorcery. Actually, it is worse than that; the current behavior doesn't just require a recompile, a make clean is required as well. Make doesn't detect that the host IP has changed. These things might be obvious to us, but if you have not read the code they are very far from that. > > Personally, I just want to go to the boot-wrapper directory, do 'make' > and be on my merry way. Not copy output from ifconfig manually and > stuff if I can avoid it. I agree with that, I just wanted to point out the potential for confusion.
I think we've covered this bit by now. On Wed, Dec 7, 2011 at 3:32 PM, Antonios Motakis <a.motakis@virtualopensystems.com> wrote: > On 12/07/2011 08:52 PM, Christoffer Dall wrote: >> >> >> In which case it would not work anyhow - the boot string is embedded >> inside the binary and a re-compile cannot be avoided. > > Yes I know, my point is that the user is not exposed to the fact that his IP > address is being used. Before, the developer did know that his IP is used in > the binary, because he put it in there himself, so he knows to do the right > thing. > > >> I think we are not talking about casual users using boot-wrapper in >> any case. It's just a question of making it easy for developers to run >> the whole setup. > > Agreed, my point is that if it is being done by a script that is called > without the user being aware of it, then even a developer might miss the > fact that the output is specific to the host IP address. > > Setting it in a config file or Makefile makes it pretty clear, even if by > default we but with backticks a command returning the current IP; however > the way it is now, where it is being called transparently from Make, it can > be confusing, because even developers are usually not used to this kind of > sorcery. > > Actually, it is worse than that; the current behavior doesn't just require a > recompile, a make clean is required as well. Make doesn't detect that the > host IP has changed. These things might be obvious to us, but if you have > not read the code they are very far from that. > > >> >> Personally, I just want to go to the boot-wrapper directory, do 'make' >> and be on my merry way. Not copy output from ifconfig manually and >> stuff if I can avoid it. > > I agree with that, I just wanted to point out the potential for confusion.
diff --git a/Makefile b/Makefile index 6dc9912..3d74ac0 100644 --- a/Makefile +++ b/Makefile @@ -35,9 +35,9 @@ all: $(IMAGE) clean: rm -f $(IMAGE) boot.o model.lds monitor.o uImage -$(KERNEL): ../linux-kvm-arm/arch/arm/boot/zImage - cd $(KERNEL_SRC); make -j4 uImage - cp $(KERNEL_SRC)/arch/arm/boot/uImage $(KERNEL) +$(KERNEL): $(KERNEL_SRC)/arch/arm/boot/uImage + $(MAKE) -C $(KERNEL_SRC) -j4 uImage + cp $< $@ $(IMAGE): boot.o monitor.o model.lds $(KERNEL) $(FILESYSTEM) Makefile $(LD) -o $@ --script=model.lds
Minor tweaks to the $(KERNEL) target: * use $(MAKE) so -n &c work better * depend on the uImage which we use, not the zImage which we don't * use automatic variables to avoid repeating the path to uImage * drop unnecessary separate cd * fix stray direct use of "../linux-kvm-arm" Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Makefile | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)