[PATCH for 4.6] build: fix tarball stubdom build

[PATCH for 4.6] build: fix tarball stubdom build

[Wei Liu]

When we create a source code tarball, mini-os is extracted to
extras/mini-os directory. When building a source code tarball, we
shouldn't clone mini-os again.

Only clone mini-os when that directory doesn't exist. This fixes tarball
build and doesn't affect non-tarball build.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 Makefile | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index e8a75ff..ba0df70 100644
--- a/Makefile
+++ b/Makefile
@@ -19,10 +19,12 @@ include Config.mk
 
 .PHONY: mini-os-dir
 mini-os-dir:
-	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
-		$(MINIOS_UPSTREAM_URL) \
-		$(MINIOS_UPSTREAM_REVISION) \
-		$(XEN_ROOT)/extras/mini-os
+	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
+		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
+			$(MINIOS_UPSTREAM_URL) \
+			$(MINIOS_UPSTREAM_REVISION) \
+			$(XEN_ROOT)/extras/mini-os ; \
+	fi
 
 .PHONY: mini-os-dir-force-update
 mini-os-dir-force-update: mini-os-dir
-- 
2.1.4

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Jan Beulich]

>>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
> When we create a source code tarball, mini-os is extracted to
> extras/mini-os directory. When building a source code tarball, we
> shouldn't clone mini-os again.
> 
> Only clone mini-os when that directory doesn't exist. This fixes tarball
> build and doesn't affect non-tarball build.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  Makefile | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e8a75ff..ba0df70 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -19,10 +19,12 @@ include Config.mk
>  
>  .PHONY: mini-os-dir
>  mini-os-dir:
> -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> -		$(MINIOS_UPSTREAM_URL) \
> -		$(MINIOS_UPSTREAM_REVISION) \
> -		$(XEN_ROOT)/extras/mini-os
> +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
> +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> +			$(MINIOS_UPSTREAM_URL) \
> +			$(MINIOS_UPSTREAM_REVISION) \
> +			$(XEN_ROOT)/extras/mini-os ; \
> +	fi

Wouldn't his better be done (avoiding the need for the shell
conditional) by simply renaming the make target from
mini-os-dir to extras/mini-os (and dropping the .PHONY)?

Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Wei Liu]

On Thu, Aug 27, 2015 at 10:05:56AM -0600, Jan Beulich wrote:
> >>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
> > When we create a source code tarball, mini-os is extracted to
> > extras/mini-os directory. When building a source code tarball, we
> > shouldn't clone mini-os again.
> > 
> > Only clone mini-os when that directory doesn't exist. This fixes tarball
> > build and doesn't affect non-tarball build.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  Makefile | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index e8a75ff..ba0df70 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -19,10 +19,12 @@ include Config.mk
> >  
> >  .PHONY: mini-os-dir
> >  mini-os-dir:
> > -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> > -		$(MINIOS_UPSTREAM_URL) \
> > -		$(MINIOS_UPSTREAM_REVISION) \
> > -		$(XEN_ROOT)/extras/mini-os
> > +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
> > +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> > +			$(MINIOS_UPSTREAM_URL) \
> > +			$(MINIOS_UPSTREAM_REVISION) \
> > +			$(XEN_ROOT)/extras/mini-os ; \
> > +	fi
> 
> Wouldn't his better be done (avoiding the need for the shell
> conditional) by simply renaming the make target from
> mini-os-dir to extras/mini-os (and dropping the .PHONY)?
> 

All targets for external trees follow some conventions defined in
tools/Makefile. Although I didn't strictly follow the same rules in
mini-os's case, I don't want it to deviate from the original rules too
much.

Wei.

> Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Ian Jackson]

Wei Liu writes ("[PATCH for 4.6] build: fix tarball stubdom build"):
> When we create a source code tarball, mini-os is extracted to
> extras/mini-os directory. When building a source code tarball, we
> shouldn't clone mini-os again.
> 
> Only clone mini-os when that directory doesn't exist. This fixes tarball
> build and doesn't affect non-tarball build.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I am about to commit this.

Thanks,
Ian.

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Jan Beulich]

>>> On 27.08.15 at 18:24, <wei.liu2@citrix.com> wrote:
> On Thu, Aug 27, 2015 at 10:05:56AM -0600, Jan Beulich wrote:
>> >>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
>> > When we create a source code tarball, mini-os is extracted to
>> > extras/mini-os directory. When building a source code tarball, we
>> > shouldn't clone mini-os again.
>> > 
>> > Only clone mini-os when that directory doesn't exist. This fixes tarball
>> > build and doesn't affect non-tarball build.
>> > 
>> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> > Cc: Ian Campbell <ian.campbell@citrix.com>
>> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> > ---
>> >  Makefile | 10 ++++++----
>> >  1 file changed, 6 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/Makefile b/Makefile
>> > index e8a75ff..ba0df70 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -19,10 +19,12 @@ include Config.mk
>> >  
>> >  .PHONY: mini-os-dir
>> >  mini-os-dir:
>> > -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
>> > -		$(MINIOS_UPSTREAM_URL) \
>> > -		$(MINIOS_UPSTREAM_REVISION) \
>> > -		$(XEN_ROOT)/extras/mini-os
>> > +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
>> > +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
>> > +			$(MINIOS_UPSTREAM_URL) \
>> > +			$(MINIOS_UPSTREAM_REVISION) \
>> > +			$(XEN_ROOT)/extras/mini-os ; \
>> > +	fi
>> 
>> Wouldn't his better be done (avoiding the need for the shell
>> conditional) by simply renaming the make target from
>> mini-os-dir to extras/mini-os (and dropping the .PHONY)?
> 
> All targets for external trees follow some conventions defined in
> tools/Makefile. Although I didn't strictly follow the same rules in
> mini-os's case, I don't want it to deviate from the original rules too
> much.

That's not true: ${target}-dir are actual directories (and hence
not phony targets) in tools/Makefile (they might also be symlinks,
but that's not of interest for the purposes here). Which is
precisely what I'm asking to be done here too, just that I'd see
the existing naming (extra/mini-os) preserved rather than
changed to extra/mini-os-dir).

Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Wei Liu]

On Fri, Aug 28, 2015 at 12:41:15AM -0600, Jan Beulich wrote:
> >>> On 27.08.15 at 18:24, <wei.liu2@citrix.com> wrote:
> > On Thu, Aug 27, 2015 at 10:05:56AM -0600, Jan Beulich wrote:
> >> >>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
> >> > When we create a source code tarball, mini-os is extracted to
> >> > extras/mini-os directory. When building a source code tarball, we
> >> > shouldn't clone mini-os again.
> >> > 
> >> > Only clone mini-os when that directory doesn't exist. This fixes tarball
> >> > build and doesn't affect non-tarball build.
> >> > 
> >> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> > Cc: Ian Campbell <ian.campbell@citrix.com>
> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> > ---
> >> >  Makefile | 10 ++++++----
> >> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >> > 
> >> > diff --git a/Makefile b/Makefile
> >> > index e8a75ff..ba0df70 100644
> >> > --- a/Makefile
> >> > +++ b/Makefile
> >> > @@ -19,10 +19,12 @@ include Config.mk
> >> >  
> >> >  .PHONY: mini-os-dir
> >> >  mini-os-dir:
> >> > -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> >> > -		$(MINIOS_UPSTREAM_URL) \
> >> > -		$(MINIOS_UPSTREAM_REVISION) \
> >> > -		$(XEN_ROOT)/extras/mini-os
> >> > +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
> >> > +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> >> > +			$(MINIOS_UPSTREAM_URL) \
> >> > +			$(MINIOS_UPSTREAM_REVISION) \
> >> > +			$(XEN_ROOT)/extras/mini-os ; \
> >> > +	fi
> >> 
> >> Wouldn't his better be done (avoiding the need for the shell
> >> conditional) by simply renaming the make target from
> >> mini-os-dir to extras/mini-os (and dropping the .PHONY)?
> > 
> > All targets for external trees follow some conventions defined in
> > tools/Makefile. Although I didn't strictly follow the same rules in
> > mini-os's case, I don't want it to deviate from the original rules too
> > much.
> 
> That's not true: ${target}-dir are actual directories (and hence
> not phony targets) in tools/Makefile (they might also be symlinks,
> but that's not of interest for the purposes here). Which is
> precisely what I'm asking to be done here too, just that I'd see
> the existing naming (extra/mini-os) preserved rather than
> changed to extra/mini-os-dir).
> 

Yeah. That's why I said "I didn't strictly follow the same rules".  To
strictly follow rules, mini-os source code should have been in
mini-os-dir (a symlink to mini-os-dir-remote or real directory
containing source code).  Target mini-os-dir should have been
mini-os-dir-find. That would cause less confusion. But because stubdom
build system is very fragile, I didn't want to touch its Makefile too
much so the name mini-os is preserved. And because mini-os doesn't
support out-of-tree build, mini-os-dir again was not necessary.

I can submit another patch to make mini-os-dir mini-os-dir-find? Other
than that trying to wire up everything to consistently follow the rules
is too much effort for too little gain.

Wei.

> Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Jan Beulich]

>>> On 28.08.15 at 10:47, <wei.liu2@citrix.com> wrote:
> On Fri, Aug 28, 2015 at 12:41:15AM -0600, Jan Beulich wrote:
>> >>> On 27.08.15 at 18:24, <wei.liu2@citrix.com> wrote:
>> > On Thu, Aug 27, 2015 at 10:05:56AM -0600, Jan Beulich wrote:
>> >> >>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
>> >> > When we create a source code tarball, mini-os is extracted to
>> >> > extras/mini-os directory. When building a source code tarball, we
>> >> > shouldn't clone mini-os again.
>> >> > 
>> >> > Only clone mini-os when that directory doesn't exist. This fixes tarball
>> >> > build and doesn't affect non-tarball build.
>> >> > 
>> >> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> >> > Cc: Ian Campbell <ian.campbell@citrix.com>
>> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> >> > ---
>> >> >  Makefile | 10 ++++++----
>> >> >  1 file changed, 6 insertions(+), 4 deletions(-)
>> >> > 
>> >> > diff --git a/Makefile b/Makefile
>> >> > index e8a75ff..ba0df70 100644
>> >> > --- a/Makefile
>> >> > +++ b/Makefile
>> >> > @@ -19,10 +19,12 @@ include Config.mk
>> >> >  
>> >> >  .PHONY: mini-os-dir
>> >> >  mini-os-dir:
>> >> > -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
>> >> > -		$(MINIOS_UPSTREAM_URL) \
>> >> > -		$(MINIOS_UPSTREAM_REVISION) \
>> >> > -		$(XEN_ROOT)/extras/mini-os
>> >> > +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
>> >> > +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
>> >> > +			$(MINIOS_UPSTREAM_URL) \
>> >> > +			$(MINIOS_UPSTREAM_REVISION) \
>> >> > +			$(XEN_ROOT)/extras/mini-os ; \
>> >> > +	fi
>> >> 
>> >> Wouldn't his better be done (avoiding the need for the shell
>> >> conditional) by simply renaming the make target from
>> >> mini-os-dir to extras/mini-os (and dropping the .PHONY)?
>> > 
>> > All targets for external trees follow some conventions defined in
>> > tools/Makefile. Although I didn't strictly follow the same rules in
>> > mini-os's case, I don't want it to deviate from the original rules too
>> > much.
>> 
>> That's not true: ${target}-dir are actual directories (and hence
>> not phony targets) in tools/Makefile (they might also be symlinks,
>> but that's not of interest for the purposes here). Which is
>> precisely what I'm asking to be done here too, just that I'd see
>> the existing naming (extra/mini-os) preserved rather than
>> changed to extra/mini-os-dir).
> 
> Yeah. That's why I said "I didn't strictly follow the same rules".  To
> strictly follow rules, mini-os source code should have been in
> mini-os-dir (a symlink to mini-os-dir-remote or real directory
> containing source code).  Target mini-os-dir should have been
> mini-os-dir-find. That would cause less confusion. But because stubdom
> build system is very fragile, I didn't want to touch its Makefile too
> much so the name mini-os is preserved. And because mini-os doesn't
> support out-of-tree build, mini-os-dir again was not necessary.
> 
> I can submit another patch to make mini-os-dir mini-os-dir-find?

Not sure that's worth it, or even consistent with the other cases.

> Other
> than that trying to wire up everything to consistently follow the rules
> is too much effort for too little gain.

I agree. But you don't really comment on the suggestion to make
the build target a real one, allowing the shell conditional to be
dropped (again, since Ian already applied your patch).

Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Wei Liu]

On Fri, Aug 28, 2015 at 03:49:06AM -0600, Jan Beulich wrote:
> >>> On 28.08.15 at 10:47, <wei.liu2@citrix.com> wrote:
> > On Fri, Aug 28, 2015 at 12:41:15AM -0600, Jan Beulich wrote:
> >> >>> On 27.08.15 at 18:24, <wei.liu2@citrix.com> wrote:
> >> > On Thu, Aug 27, 2015 at 10:05:56AM -0600, Jan Beulich wrote:
> >> >> >>> On 27.08.15 at 17:54, <wei.liu2@citrix.com> wrote:
> >> >> > When we create a source code tarball, mini-os is extracted to
> >> >> > extras/mini-os directory. When building a source code tarball, we
> >> >> > shouldn't clone mini-os again.
> >> >> > 
> >> >> > Only clone mini-os when that directory doesn't exist. This fixes tarball
> >> >> > build and doesn't affect non-tarball build.
> >> >> > 
> >> >> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> >> > Cc: Ian Campbell <ian.campbell@citrix.com>
> >> >> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> >> > ---
> >> >> >  Makefile | 10 ++++++----
> >> >> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >> >> > 
> >> >> > diff --git a/Makefile b/Makefile
> >> >> > index e8a75ff..ba0df70 100644
> >> >> > --- a/Makefile
> >> >> > +++ b/Makefile
> >> >> > @@ -19,10 +19,12 @@ include Config.mk
> >> >> >  
> >> >> >  .PHONY: mini-os-dir
> >> >> >  mini-os-dir:
> >> >> > -	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> >> >> > -		$(MINIOS_UPSTREAM_URL) \
> >> >> > -		$(MINIOS_UPSTREAM_REVISION) \
> >> >> > -		$(XEN_ROOT)/extras/mini-os
> >> >> > +	if [ ! -d $(XEN_ROOT)/extras/mini-os ]; then \
> >> >> > +		GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh \
> >> >> > +			$(MINIOS_UPSTREAM_URL) \
> >> >> > +			$(MINIOS_UPSTREAM_REVISION) \
> >> >> > +			$(XEN_ROOT)/extras/mini-os ; \
> >> >> > +	fi
> >> >> 
> >> >> Wouldn't his better be done (avoiding the need for the shell
> >> >> conditional) by simply renaming the make target from
> >> >> mini-os-dir to extras/mini-os (and dropping the .PHONY)?
> >> > 
> >> > All targets for external trees follow some conventions defined in
> >> > tools/Makefile. Although I didn't strictly follow the same rules in
> >> > mini-os's case, I don't want it to deviate from the original rules too
> >> > much.
> >> 
> >> That's not true: ${target}-dir are actual directories (and hence
> >> not phony targets) in tools/Makefile (they might also be symlinks,
> >> but that's not of interest for the purposes here). Which is
> >> precisely what I'm asking to be done here too, just that I'd see
> >> the existing naming (extra/mini-os) preserved rather than
> >> changed to extra/mini-os-dir).
> > 
> > Yeah. That's why I said "I didn't strictly follow the same rules".  To
> > strictly follow rules, mini-os source code should have been in
> > mini-os-dir (a symlink to mini-os-dir-remote or real directory
> > containing source code).  Target mini-os-dir should have been
> > mini-os-dir-find. That would cause less confusion. But because stubdom
> > build system is very fragile, I didn't want to touch its Makefile too
> > much so the name mini-os is preserved. And because mini-os doesn't
> > support out-of-tree build, mini-os-dir again was not necessary.
> > 
> > I can submit another patch to make mini-os-dir mini-os-dir-find?
> 
> Not sure that's worth it, or even consistent with the other cases.
> 
> > Other
> > than that trying to wire up everything to consistently follow the rules
> > is too much effort for too little gain.
> 
> I agree. But you don't really comment on the suggestion to make
> the build target a real one, allowing the shell conditional to be
> dropped (again, since Ian already applied your patch).
> 

Didn't mean to ignore that.

Yes. Using real target is one way of doing it. Feel free to submit patch
to change to that after 4.7 window opens. I've also added that to my
list but its priority is very low.

Wei.

> Jan

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 703 bytes --]

On 8/27/15 12:58 PM, Ian Jackson wrote:
> Wei Liu writes ("[PATCH for 4.6] build: fix tarball stubdom build"):
>> When we create a source code tarball, mini-os is extracted to
>> extras/mini-os directory. When building a source code tarball, we
>> shouldn't clone mini-os again.
>>
>> Only clone mini-os when that directory doesn't exist. This fixes tarball
>> build and doesn't affect non-tarball build.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I am about to commit this.
> 
> Thanks,
> Ian.

Ping. Do we want this to land for 4.6? Since all its doing is
documenting the Makefile targets in the actual 4.6 Makefile I would
think its safe.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH for 4.6] build: fix tarball stubdom build

[Ian Campbell]

On Fri, 2015-09-04 at 08:48 -0500, Doug Goldstein wrote:
> On 8/27/15 12:58 PM, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH for 4.6] build: fix tarball stubdom build"):
> > > When we create a source code tarball, mini-os is extracted to
> > > extras/mini-os directory. When building a source code tarball, we
> > > shouldn't clone mini-os again.
> > > 
> > > Only clone mini-os when that directory doesn't exist. This fixes 
> > > tarball
> > > build and doesn't affect non-tarball build.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > I am about to commit this.
> > 
> > Thanks,
> > Ian.
> 
> Ping. Do we want this to land for 4.6?

It is:

commit 0cc73e9870a96e18fc076618c0b419919794ae06
Author: Wei Liu <wei.liu2@citrix.com>
Date:   Thu Aug 27 16:54:01 2015 +0100

    build: fix tarball stubdom build

>  Since all its doing is
> documenting the Makefile targets in the actual 4.6 Makefile I would
> think its safe.

But that makes me think you hit rpely on the wrong email.

If you meant "build: update top-level make help" then that is mentioned in 
http://lists.xen.org/archives/html/xen-devel/2015-09/msg00533.html (and
I've not read Wei's reply yet)

Ian.