| Message ID | 20190510203452.11870-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | configure: Disable slirp if --disable-system | expand |
On 10/05/2019 22.34, Richard Henderson wrote: > For linux-user, there is no need to add slirp to the set of > git modules checked out, nor build it. > > This also avoids a makefile bug wrt insufficient dependencies > on subdir-slirp. If slirp/ is not initially present, the > dependencies that check it out are associated with softmmu, > which then generates a build error on slirp/ not present. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > configure | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/configure b/configure > index 63f312bd1f..9de7e43a80 100755 > --- a/configure > +++ b/configure > @@ -5878,6 +5878,10 @@ fi > ########################################## > # check for slirp > > +if test "$softmmu" = "no"; then > + slirp=no > +fi Maybe also check that the user did not try to run configure with both, --disable-system and --enable-slirp? I.e. that $slirp != "yes" ? Thomas
On May 10, 2019 10:36 PM, "Richard Henderson" <richard.henderson@linaro.org> wrote: > > For linux-user, there is no need to add slirp to the set of > git modules checked out, nor build it. > > This also avoids a makefile bug wrt insufficient dependencies > on subdir-slirp. If slirp/ is not initially present, the > dependencies that check it out are associated with softmmu, > which then generates a build error on slirp/ not present. > Hi, Does this work if only user mode targets are specified via ˊ--target-listˊ switch? If no, the patch shoud be amended. If yes, the commit message should be extended. Thanks, Aleksandar > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > configure | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/configure b/configure > index 63f312bd1f..9de7e43a80 100755 > --- a/configure > +++ b/configure > @@ -5878,6 +5878,10 @@ fi > ########################################## > # check for slirp > > +if test "$softmmu" = "no"; then > + slirp=no > +fi > + > case "$slirp" in > "" | yes) > if $pkg_config slirp; then > -- > 2.17.1 > >
On 5/11/19 5:47 AM, Aleksandar Markovic wrote: > > On May 10, 2019 10:36 PM, "Richard Henderson" <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> wrote: >> >> For linux-user, there is no need to add slirp to the set of >> git modules checked out, nor build it. >> >> This also avoids a makefile bug wrt insufficient dependencies >> on subdir-slirp. If slirp/ is not initially present, the >> dependencies that check it out are associated with softmmu, >> which then generates a build error on slirp/ not present. >> > > Hi, > > Does this work if only user mode targets are specified via ˊ--target-listˊ > switch? Yes. There is a bit of code that converts such a target list to the same result as --disable-system, which is $softmmu = no. > If no, the patch shoud be amended. If yes, the commit message should be > extended. Like what? I think it's pretty clear as is. r~
On May 13, 2019 11:14 PM, "Richard Henderson" <richard.henderson@linaro.org> wrote: > > On 5/11/19 5:47 AM, Aleksandar Markovic wrote: > > > > On May 10, 2019 10:36 PM, "Richard Henderson" < richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> wrote: > >> > >> For linux-user, there is no need to add slirp to the set of > >> git modules checked out, nor build it. > >> > >> This also avoids a makefile bug wrt insufficient dependencies > >> on subdir-slirp. If slirp/ is not initially present, the > >> dependencies that check it out are associated with softmmu, > >> which then generates a build error on slirp/ not present. > >> > > > > Hi, > > > > Does this work if only user mode targets are specified via ˊ--target-listˊ > > switch? > > Yes. There is a bit of code that converts such a target list to the same > result as --disable-system, which is $softmmu = no. > > > If no, the patch shoud be amended. If yes, the commit message should be > > extended. > > Like what? I think it's pretty clear as is. > Richard, no. In this case, there is a glaring discrepancy between the title and the functionality that the change provides. Much better title would be “configure: Disable slirp if no system mode target is selected”. I leave it to you to find out what can be improved in the commit message. How well did you test your change? Did you try some corner cases? I don't have concerns about the wording of the commit message only. I agree with Thomas that combination of “no system mode target is selected” and “--enable-slirp is used” must have some special handing. We can't just leave the rest of the script to do whatever the current code happens to do. The patch code should be completed. Thanks, Aleksandar > > r~
On Tue, 14 May 2019 at 20:16, Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote: > > On May 13, 2019 11:14 PM, "Richard Henderson" <richard.henderson@linaro.org> > wrote: > > > > On 5/11/19 5:47 AM, Aleksandar Markovic wrote: > > > If no, the patch shoud be amended. If yes, the commit message should be > > > extended. > > > > Like what? I think it's pretty clear as is. > > > > Richard, no. In this case, there is a glaring discrepancy between the title > and the functionality that the change provides. Much better title would be > “configure: Disable slirp if no system mode target is selected”. > > I leave it to you to find out what can be improved in the commit message. Aleksandar: I think this is not really a very productive stance to take. Richard thinks the commit message is reasonable. If you have something you would like him to change, I think we will reach a useful endpoint much more quickly and smoothly if you suggest some new text, rather than effectively saying "you need to think of something, and I'm going to keep making you rewrite it until you telepathically figure out what the text I wanted you to write is". thanks -- PMM
On May 15, 2019 12:07 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > > On Tue, 14 May 2019 at 20:16, Aleksandar Markovic > <aleksandar.m.mail@gmail.com> wrote: > > > > On May 13, 2019 11:14 PM, "Richard Henderson" < richard.henderson@linaro.org> > > wrote: > > > > > > On 5/11/19 5:47 AM, Aleksandar Markovic wrote: > > > > If no, the patch shoud be amended. If yes, the commit message should be > > > > extended. > > > > > > Like what? I think it's pretty clear as is. > > > > > > > Richard, no. In this case, there is a glaring discrepancy between the title > > and the functionality that the change provides. Much better title would be > > “configure: Disable slirp if no system mode target is selected”. > > > > I leave it to you to find out what can be improved in the commit message. > > Aleksandar: I think this is not really a very productive stance to take. > Richard thinks the commit message is reasonable. If you have something > you would like him to change, I think we will reach a useful endpoint > much more quickly and smoothly if you suggest some new text, rather than > effectively saying "you need to think of something, and I'm going to keep > making you rewrite it until you telepathically figure out what the text > I wanted you to write is". > OK, Peter, no problem from my side. I was trying to make Richard think more about what he writes in his commit messages, and how he organizes his code. Sorry if this looked unproductive or even perhaps offensive. Yours, Aleksadar > thanks > -- PMM
diff --git a/configure b/configure index 63f312bd1f..9de7e43a80 100755 --- a/configure +++ b/configure @@ -5878,6 +5878,10 @@ fi ########################################## # check for slirp +if test "$softmmu" = "no"; then + slirp=no +fi + case "$slirp" in "" | yes) if $pkg_config slirp; then
For linux-user, there is no need to add slirp to the set of git modules checked out, nor build it. This also avoids a makefile bug wrt insufficient dependencies on subdir-slirp. If slirp/ is not initially present, the dependencies that check it out are associated with softmmu, which then generates a build error on slirp/ not present. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- configure | 4 ++++ 1 file changed, 4 insertions(+) -- 2.17.1