Re: [PATCH 2/3] archiver.bbclass: archive sources, patches, logs to tarball

[Mark Hatle <mark.hatle@windriver.com>]

Apologies for the very late review...  see comments below.  All in all it looks 
good.  But I have not applied it and tried it yet.

On 3/3/12 4:54 AM, Xiaofeng Yan wrote:
> From: Xiaofeng Yan<xiaofeng.yan@windriver.com>
>
> Support the following functions in this bbclass:
>
> 1 Archive sources in ${S} in the different stage to tarball (do_unpack,do_patch,do_configure).
> 2 Archive patches including series to tarball
> 3 Archive logs including scripts (.bb and .inc files) to tarball
> 4 dump environment resources which show all variable and functions to be
>    used to xxx.showdata.dump when running a task
> 5 dump all content in 's' including patches to file xxx.diff.gz
>
> All of tarballs will be deployed to ${DEPLOY_DIR}/sources/
>
> [#YOCTO 1977]
>
> Signed-off-by: Xiaofeng Yan<xiaofeng.yan@windriver.com>
> ---
>   meta/classes/archiver.bbclass |  393 +++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 393 insertions(+), 0 deletions(-)
>   create mode 100644 meta/classes/archiver.bbclass
>
...

> +def verify_var(d):
> +	'''check the type for archiving package('tar' or 'srpm')'''
> +	try:
> +		if d.getVar('SOURCE_ARCHIVE_PACKAGE_TYPE', True).upper() not in d.getVar('ARCHIVE_TYPE', True).split():
> +			raise AttributeError	
> +	except AttributeError:
> +			bb.fatal("\"SOURCE_ARCHIVE_PACKAGE_TYPE\" is \'tar\' or \'srpm\', no other types")

Error message should be something like:

bb.fatal("\"SOURCE_ARCHIVE_PACKAGE_TYPE\" should be \'tar\' or '\srpm\'")

...

> +def archive_sources_patches(d,middle_name):
> +	'''archive sources and patches to tarball. middle_name will append strings ${middle_name} to ${PR} as middle name. for example, zlib-1.4.6-prepatch(middle_name).tar.gz '''
> +	verify_var(d)	
> +	if not_tarball(d):
> +		return
> +	
> +	source_tar_name = archive_sources(d,middle_name)
> +	if middle_name == "prepatch":
> +		if d.getVar('PATCHES_ARCHIVE_WITH_SERIES',True).upper() == 'TRUE':
> +			patch_tar_name = select_archive_patches(d,"all")
> +		elif d.getVar('PATCHES_ARCHIVE_WITH_SERIES',True).upper() == 'FALSE':
> +			patch_tar_name = select_archive_patches(d,"applying")
> +		else:
> +			bb.fatal("Please define 'PATCHES_ARCHIVE_WITH_SERIES' is strings 'True' or 'False' ")

Instead of an explicit TRUE/FALSE setting, does it make sense for one to be the 
default?  I'd suspect in this case "True" is the better default, but I'm not 
completely sure.

> +	else:
> +		patch_tar_name = ''
> +
> +	if d.getVar('SOURCE_ARCHIVE_PACKAGE_TYPE', True).upper() not in 'SRPM':
> +		move_tarball_deploy(d,[source_tar_name,patch_tar_name])
> +
> +def archive_sources_patches_logs_copyleft(d,middle_name):
> +	'''archive source, patches and logs according to the variable "COPYLEFT_COMPLIANCE", If this variable is 'True', then archive the packages for copy-left, or else archive all packages'''
> +	verify_var(d)	
> +	if not_tarball(d):
> +		return
> +	copyleft_compliance = d.getVar('COPYLEFT_COMPLIANCE', True)
> +	if copyleft_compliance is None:
> +		archive_sources_patches(d,middle_name)
> +	elif copyleft_compliance.upper() == 'TRUE' and archive_copyleft(d):
> +		archive_sources_patches(d,middle_name)

I'm not sure I understand what is happening in this patch.  The way I read it:

If COPYLEFT_COMPLIANCE is -not- set, then we archive..  otherwise if it is set 
(true), and the copyleft item was inherited we also archive?

But I'm not sure I understand why this set of checks.


...

> +def dumpdata(d):
> +	'''dump environment to "${P}-${PR}.showdata.dump" including all kinds of varibale and functions when running a task'''

Simple typo above, should be "variable".

...

> +# This functions prepare for archiving "linux-yocto" because this package create directory 's' before do_patch instead of after do_unpack.
> +# This is special control for archiving linux-yocto only.
> +python do_archive_linux_yocto(){
> +	s = d.getVar('S', True)
> +	if 'linux-yocto' in s:
> +		source_tar_name = archive_sources(d,'')
> +	if d.getVar('SOURCE_ARCHIVE_PACKAGE_TYPE', True).upper() not in 'SRPM':
> +		move_tarball_deploy(d,[source_tar_name,''])
> +}

Is there something we can change in linux-yocto to make the standard methods 
work?  I'm hesitant to put special logic in the archiver class for a single 
package.  (Note, I'm more likely to think it's reasonable for the kernel's 
package then a random userspace package!)

> +do_kernel_checkout[postfuncs] += "do_archive_linux_yocto "
> +

Is the real issue that we want to run something just before do_patch, but we 
also want to let any arbitrary tasks between do_unpack and do_patch to run 
first?  Is there an alternative way to state this?  [or at least detect a 
situation where we haven't waited?]

> +# remove tarball for sources, patches and logs after creating srpm.
> +python do_remove_tarball(){
> +	if d.getVar('SOURCE_ARCHIVE_PACKAGE_TYPE', True).upper() == 'SRPM':
> +		work_dir = d.getVar('WORKDIR', True)
> +		os.chdir(work_dir)
> +		for file in os.listdir(os.getcwd()):
> +			if '.tar.gz' in file:
> +				os.remove(file)
> +}
> +do_remove_taball[deptask] = "do_archive_scripts_logs"
> +do_package_write_rpm[postfuncs] += "do_remove_tarball "

Finally do we really need to remove the tarball?  It can likely just stay 
around, when the user wipes out/cleans the WORKDIR it will go away on it's own?

--Mark