[Buildroot] [RFCv1 4/4] core: implement per-package SDK and target

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Hello,

Thanks a lot for the suggestions, I'll take them into account for the
next version. Just a few comments/questions below.

On Tue, 7 Nov 2017 23:50:52 +0100, Arnout Vandecappelle wrote:

> > In order to fix this, this commit extends the target-finalize step so
> > that it starts by populating output/target and output/host by
> > rsync-ing into them the target and host directories of all packages
> > listed in the $(PACKAGES) variable.  
> 
>  It would have been good to mention somewhere in the commit log the problem of 2
> packages writing the same file...

True. I believe the work Yann has done to detect packages overwriting
files installed by other packages is definitely a pre-requisite for
this work, so I was kind of assuming that the "packages don't overwrite
each other" would already be a rule by the time my per-package SDK
patches get merged. But I should make this explicit in the cover letter
at least.

>  Also, is there a good reason not to do the rsync at the end of package
> installation? I don't know how robust rsync is against race conditions in
> directory or hardlink creation... OK, so I did a test and it races pretty badly
> :-) So, could you add this to the commit log? Like:
> 
> It is necessary to do this sequentially in the target-finalize step and not in
> each package. Doing it in package installation means that it can be done in
> parallel. In that case, there is a chance that two rsyncs are creating the same
> hardlink or directory at the same time, which makes one of them fail.

I didn't think at all about doing this in parallel. To me, the creation
of the final/common HOST_DIR/TARGET_DIR/STAGING_DIR is not at
all something that needs to be done while the build is on-going. And I
could go even further: unless you call "make sdk", we technically don't
really need to create the final/common HOST_DIR/STAGING_DIR. Only a
TARGET_DIR is needed, and perhaps even we could avoid a global one, and
instead have a per-filesystem image one, in order to avoid the parallel
filesystem image creation problem.

> > [Note: in fact, output/host is not completely empty, even though they
> > should be. It contains the following files:
> > output/host/
> > output/host/lib64
> > output/host/usr
> > output/host/share
> > output/host/share/buildroot
> > output/host/share/buildroot/Platform
> > output/host/share/buildroot/Platform/Buildroot.cmake
> > output/host/share/buildroot/toolchainfile.cmake
> > output/host/lib
> > Perhaps those files should be installed by some package instead,
> > perhaps the 'toolchain' package.]  
> 
>  If we go that way: perhaps the .cmake files should be installed by some package
> like 'host-cmake-infra' that is added to the dependencies from cmake-package.

That was my thinking: perhaps those files should all be installed by
packages.

> >  .PHONY: target-finalize
> >  target-finalize: $(PACKAGES)
> > +	@$(call MESSAGE,"Creating global target directory")
> > +	$(foreach dir,$(ALL_PACKAGES_TARGET_DIRS),\
> > +		rsync -au --link-dest=$(dir) $(dir) $(TARGET_DIR)$(sep))  
> 
>  Why -u? Since we're not actually copying anything, there is no reason to not
> overwrite files that already exist, right?

I just borrowed parts of Gustavo patches, including those rsync
invocations. Indeed -u is not needed.

>  Also, shouldn't there be a / behind $(dir)? Otherwise, if TARGET_DIR exists
> already, rsync will create $(dir) as a subdirectory of $(TARGET_DIR), no?

I'll fix.

> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 82f8c06821..aee4011f63 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -158,6 +158,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
> >  $(BUILD_DIR)/%/.stamp_extracted:
> >  	@$(call step_start,extract)
> >  	@$(call MESSAGE,"Extracting")
> > +	@mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR)  
> 
>  Why do you create them here? It would make more sense to do it just before the
> rsync, no?

I don't remember, I'll have to check again. I don't immediately see a
reason why they would be needed before the rsync.

>  Given the possible host-tar and host-lzip dependencies, the rsync should be
> done here already, not in the configure step. Ouch, that's not good, because the
> dependencies are only evaluated for the configure step... There are
> PATCH_DEPENDENCIES, but those are only before patch. Well, I guess that's a
> reason to keep host-tar and host-lzip as DEPENDENCIES_HOST_PREREQ :-)

These are all topics that I haven't looked at yet. So thanks for
pointing them out. I should probably collect a TODO list on per-package
SDK to not forget about all the things people have mentioned as things
to look at.

> > +	$(foreach dir,$($(PKG)_FINAL_DEPENDENCIES_HOST_DIRS),\
> > +		rsync -au --link-dest=$(dir) $(dir) $(HOST_DIR)$(sep))  
> 
>  Shouldn't there be a $Q somewhere?

Yes. Seeing the rsync commands was useful for debugging, as you can
imagine :)


> > +$(2)_TARGET_DIR		= $$(PER_PACKAGE_DIR)/$(1)/target/  
> 
>  Ah, here is the trailing slash, that explains why it worked. I don't really
> like it when you have to rely on a trailing slash in the variable definition...

OK.

> 
> > +$(2)_HOST_DIR		= $$(PER_PACKAGE_DIR)/$(1)/host/
> > +$(2)_STAGING_DIR	= $$(PER_PACKAGE_DIR)/$(1)/host/$(STAGING_SUBDIR)  
> 
>  I hate adding more per-package variables. Can't you use the expanded values,
> substituting $($(PKG)_NAME) for $(1)? Or maybe even better, change the
> definition of TARGET_DIR etc:
> 
> TARGET_DIR = $(if
> $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_RAWNAME)/target,$(BASE_DIR)/target)
> 
>  That way you also don't need to pass those variables to each individual stamp rule.

I didn't think about this, sounds like a good idea, I'll try that.

However, I'm actually planning to drop entirely having TARGET_DIR
defined outside of package rules. What I want to have ultimately is:

 - TARGET_DIR points to the current package per-package target
   directory. If we're outside of a package rule, TARGET_DIR has no
   value (or possibly a bogus value such as /this/is/a/bogus/thing/).
   Indeed, we really don't want things to directly install/mess with
   the global/common target directory.

 - Have something like COMMON_TARGET_DIR that points to
   $(BASE_DIR)/target, and we carefully change the places that should
   access the COMMON_TARGET_DIR (i.e mainly target-finalize and some
   filesystem generation stuff).

> >  # Keep the package version that may contain forward slashes in the _DL_VERSION
> >  # variable, then replace all forward slashes ('/') by underscores ('_') to
> >  # sanitize the package version that is used in paths, directory and file names.
> > @@ -561,6 +570,13 @@ $(2)_FINAL_DEPENDENCIES = $$(sort $$($(2)_DEPENDENCIES))
> >  $(2)_FINAL_PATCH_DEPENDENCIES = $$(sort $$($(2)_PATCH_DEPENDENCIES))
> >  $(2)_FINAL_ALL_DEPENDENCIES = $$(sort $$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES))
> >  
> > +$(2)_FINAL_DEPENDENCIES_HOST_DIRS = \
> > +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> > +		$$($$(call UPPERCASE,$$(dep))_HOST_DIR))
> > +$(2)_FINAL_DEPENDENCIES_TARGET_DIRS = \
> > +	$$(foreach dep,$$($(2)_FINAL_DEPENDENCIES),\
> > +		$$($$(call UPPERCASE,$$(dep))_TARGET_DIR))  
> 
>  I may be overengineering things, but perhaps things can be simplified as
> follows (with the definition of TARGET_DIR etc as above):
> 
> define RSYNC_HOST_DIRS
> 	$(foreach pkg,$(1),\
> 		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/host;\
> 		rsync -au --link-dest=$$dir $$dir/ $(HOST_DIR)$(sep))
> endef
> 
> define RSYNC_TARGET_DIRS
> 	$(foreach pkg,$(1),\
> 		$(Q)dir=$(PER_PACKAGE_DIR)/$(pkg)/target;\
> 		rsync -au --link-dest=$$dir $$dir/ $(TARGET_DIR)$(sep))
> endef
> 
> (as always, completely untested). These macros can be used both in the config
> stamp rule and in the finalize rule. I even considered leaving out the argument
> and instead defaulting $(1) to
> $(if $(PKG),$($(PKG)_FINAL_DEPENDENCIES),$(PACKAGES))
> but that probably just makes things harder to understand.

I'll think about this suggestion.

Thanks again for the detailed review.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com