| Message ID | 20200117105358.607910-6-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 17.01.2020 11:53, Anthony PERARD wrote: > It isn't necessary to include Config.mk here because this Makefile is > only used by xen/Rules.mk which already includes Config.mk. And so is xen/test/livepatch/Makefile afaics from its parent dir Makefile. With this also adjusted (or it explained why I'm seeing things incorrectly) ... > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 29.01.2020 16:28, Jan Beulich wrote: > On 17.01.2020 11:53, Anthony PERARD wrote: >> It isn't necessary to include Config.mk here because this Makefile is >> only used by xen/Rules.mk which already includes Config.mk. > > And so is xen/test/livepatch/Makefile afaics from its parent dir > Makefile. With this also adjusted (or it explained why I'm seeing > things incorrectly) ... > >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> And now I've seen that patch 6 does just this. I think such common theme changes are, unless patches are overly large already, better put all in on patch. Anyway - the R-b then is unconditional. Another question: The cover letter doesn't say anything about some (or most) patches here being independent of one another, and hence the option of them going in out of order. The one here looks to be entirely standalone, for example. Jan
On Wed, Jan 29, 2020 at 04:33:02PM +0100, Jan Beulich wrote: > On 29.01.2020 16:28, Jan Beulich wrote: > > On 17.01.2020 11:53, Anthony PERARD wrote: > >> It isn't necessary to include Config.mk here because this Makefile is > >> only used by xen/Rules.mk which already includes Config.mk. > > > > And so is xen/test/livepatch/Makefile afaics from its parent dir > > Makefile. With this also adjusted (or it explained why I'm seeing > > things incorrectly) ... > > > >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > And now I've seen that patch 6 does just this. I think such > common theme changes are, unless patches are overly large > already, better put all in on patch. Anyway - the R-b then > is unconditional. I don't like squashing unrelated changes together. I though both changes deserved there own explanation in this case. They don't touch the same subsystem, they don't have the same set of maintainers. > Another question: The cover letter doesn't say anything about > some (or most) patches here being independent of one another, > and hence the option of them going in out of order. The one > here looks to be entirely standalone, for example. It is extra work to figure out which patch could be applied out of order. I would have independent patch at the beginning of the series, but if there aren't, it is probably because I haven't though they were important enough to think about applying them independently. I might try to reorder some patches in later version of a series to have them applied earlier. As for this series, I do think applying most patches in order is important, changing the order may lead to unexpected breakage. That might not be true, but I don't want to spend time on checking that. Cheers,
On 30.01.2020 19:34, Anthony PERARD wrote: > On Wed, Jan 29, 2020 at 04:33:02PM +0100, Jan Beulich wrote: >> On 29.01.2020 16:28, Jan Beulich wrote: >>> On 17.01.2020 11:53, Anthony PERARD wrote: >>>> It isn't necessary to include Config.mk here because this Makefile is >>>> only used by xen/Rules.mk which already includes Config.mk. >>> >>> And so is xen/test/livepatch/Makefile afaics from its parent dir >>> Makefile. With this also adjusted (or it explained why I'm seeing >>> things incorrectly) ... >>> >>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >>> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> And now I've seen that patch 6 does just this. I think such >> common theme changes are, unless patches are overly large >> already, better put all in on patch. Anyway - the R-b then >> is unconditional. > > I don't like squashing unrelated changes together. I though both changes > deserved there own explanation in this case. They don't touch the same > subsystem, they don't have the same set of maintainers. Yes, the issue was in part because I noticed too late that there was a 2nd, similar patch (and hence I went and checked whether you've caught all instances where removal would be possible). I understand your concerns, yet I think these two aren't unrelated. Under a title "remove unnecessary includes of Config.mk" both would have fit. But don't get me wrong, I'm fine with them remaining split. A post-commit-message remark clarifying this doesn't cover all instances would have helped review nevertheless. >> Another question: The cover letter doesn't say anything about >> some (or most) patches here being independent of one another, >> and hence the option of them going in out of order. The one >> here looks to be entirely standalone, for example. > > It is extra work to figure out which patch could be applied out of > order. I would have independent patch at the beginning of the series, > but if there aren't, it is probably because I haven't though they were > important enough to think about applying them independently. I might try > to reorder some patches in later version of a series to have them > applied earlier. > > As for this series, I do think applying most patches in order is > important, changing the order may lead to unexpected breakage. That > might not be true, but I don't want to spend time on checking that. Fair enough. With my committer hat on, I just typically try to spot opportunities of committing pieces, to reduce the overall volume of to-be-resent patches. Jan
diff --git a/xen/include/Makefile b/xen/include/Makefile index fde0ca013121..433bad9055b2 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -1,5 +1,3 @@ -include $(XEN_ROOT)/Config.mk - ifneq ($(CONFIG_COMPAT),) compat-arch-$(CONFIG_X86) := x86_32
It isn't necessary to include Config.mk here because this Makefile is only used by xen/Rules.mk which already includes Config.mk. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- xen/include/Makefile | 2 -- 1 file changed, 2 deletions(-)