Hi,
>> @@ -1354,6 +1356,11 @@ parse_first()
>> --git)
>> # overwrite default from ptxdistrc
>> export PTXCONF_SETUP_PATCHIN_GIT=y
>> + export PTXCONF_SETUP_PATCHIN_GIT_PKG_LIST="*"
>
> Hmm, maybe a --git="kernel barebox" would be nice...
Agreed, but I thought to not make it to fancy in the first patch ;-)
> An idea here: force git for packages in PTXCONF_SETUP_PATCHIN_GIT_PKG_LIST
> (if it's not *) even if there are no patches. Starting the first patch is
> not very easy right now. Maybe use ${PTXDIST_TOPDIR}/patches/ if ptxdist is
> a not release version and the directory is writeable. Otherwise fall back
> to ${PTXDIST_WORKSPACE}/patches
> What do you think?
100% agreed. I thought about that too, but again not trying to make it
too fancy in the first run.
Note that it could also be an option to always start with the ptxdist
patches, followed by the project defined patches.
We tend to keep on copying the ptxdist patches to the project patches
directory if we want to add something new, and we tend to forget to
synchronise these copied patches when we update ptxdist...
>> +# $(1): options to be passed to the touch command
>> touch = \
>> target="$(strip $(@))"; \
>> - touch "$${target}"; \
>> + touch "$${target}" $(1); \
>> target="$${target\#\#*/}"; \
>> $(_touch_opt_output) \
>> echo "finished target $${target}"
>
> I don't like this. We had a parameter for touch before. And there still
> Makefiles with it out there. I don't think it breaks with those, but it's
> still not nice.
hmm, I happened to like this trick ;-))
>> diff --git a/rules/post/ptxd_make_world_patchin.make
>> b/rules/post/ptxd_make_world_patchin.make
>> index 48c72a1..39c9652 100644
>> --- a/rules/post/ptxd_make_world_patchin.make
>> +++ b/rules/post/ptxd_make_world_patchin.make
>
> This should go to rules/post/ptxd_make_world_update.make
I expected this remark already ;-)
>> @@ -18,4 +18,36 @@ patchin = \
>> pkg_deprecated_patchin_series="$(call ptx/escape,$(3))" \
>> $(call world/patchin, $(1))
>>
>> +world/update = \
>> + pkg_target_update=$@; \
>> + pkg_target_extract=$${pkg_target_update/%update/extract}; \
>
> I don't like this here. Maybe define it as
> ${ptx_state_dir}/${pkg_label}.<stage> in the script.
OK
>> + pkg_deprecated_patchin_dir="$(call ptx/escape,$(2))" \
>> + pkg_deprecated_patchin_series="$(call ptx/escape,$(3))" \
>
> These are named _deprecated_ for a reason. They only exist in patchin for
> backwards compatibility.
I did not understood exactly why it was there in the first place, but
we call the patchin scripts from within the update stage. So, I
figured it would not hurt to add it.
>> + $(call world/env, $(1)) \
>> + pkg_patch_series="$(call ptx/escape,$(call remove_quotes,
>> $(PTXCONF_$(strip $(1))_SERIES)))" \
>
> This should be in ptxd_make_world_common.make. A separate patch to move it
> please.
OK
>> + ptxd_make_world_update
>> +
>> +update = \
>> + $(call targetinfo) \
>> + $(call world/update,$(1),$($(1)_DIR)); \
>> + if [ $$? -eq 0 ]; then \
>> + $(call touch,-r $@); \
>> + else \
>> + echo "package not up-to-date"; \
>> + $(call touch); \
>> + fi
>
> Not sure I like this. The return value should be reserved to indicate
> failure.
OK
>> +
>> +# The update target is dependant on FORCE to force it to be called on every
>> +# incremental 'ptxdist go'. It checks for changed patches, and if some
>> patch has
>> +# changed, it will update the *.update target. This update triggers the
>> rebuild
>> +# of prepare, compile and so on.
>> +
>> +### --- for KLIBC packages only ---
>> +$(STATEDIR)/klibc-%.update: FORCE
>> + @$(call update,$(PTX_MAP_TO_PACKAGE_klibc-$(*)))
>> +
>> +### --- all but KLIBC packages ---
>> +$(STATEDIR)/%.update: FORCE
>> + @$(call update,$(PTX_MAP_TO_PACKAGE_$(*)))
>
> This means _always_ running ptxd_make_world_update for all packages. I
> don't like that.
>
> How about this: for all packages that should be updated:
> - create (in dgen) a target that checks if updating is necessary
> - if yes then delete $(STATEDIR)/<pkg>.update
> -force it and have $(STATEDIR)/<pkg>.update depend on it.
> --> ptxd_make_world_update is called when necessary.
> --> no ugly touch tricks needed
>
> or create a generic check stage and only add the dependency for the
> relevant packages.
I have to look into that.
>> clean)
>> COMPREPLY=( $( compgen -W "${opts} $( ptxdist print
>> PTX_PACKAGES_SELECTED ) root" -- $cur ) )
>> ;;
>> drop)
>> - COMPREPLY=( $( compgen -W "${opts} $( pushd $(ptxdist print
>> PTXDIST_PLATFORMDIR)/state >/dev/null; ls
>> +(*.get|*.extract|*.prepare|*.compile|*.install|*.targetinstall); popd
>> >/dev/null )" -- $cur ) )
>> + COMPREPLY=( $( compgen -W "${opts} $( pushd $(ptxdist print
>> PTXDIST_PLATFORMDIR)/state >/dev/null; ls
>> +(*.get|*.extract|*.update|*.prepare|*.compile|*.install|*.targetinstall);
>> popd >/dev/null )" -- $cur ) )
>> ;;
>> newpackage)
>> COMPREPLY=( $( compgen -W "${opts} target host cross klibc
>> arc-autoconf-lib src-autoconf-prog src-autoconf-proglib src-cmake-prog
>> src-linux-dirver src-make-prog src-stellaris font simple" -- $cur) )
>
> separate patch for bash_completion please.
OK
>> diff --git a/scripts/lib/ptxd_lib_dgen.awk b/scripts/lib/ptxd_lib_dgen.awk
>> index d446412..3583731 100644
>> --- a/scripts/lib/ptxd_lib_dgen.awk
>> +++ b/scripts/lib/ptxd_lib_dgen.awk
>> @@ -300,7 +300,8 @@ END {
>> # default deps
>> #
>> print "$(STATEDIR)/" this_pkg ".extract: "
>> "$(STATEDIR)/" this_pkg ".get" > DGEN_DEPS_POST;
>> - print "$(STATEDIR)/" this_pkg ".prepare: "
>> "$(STATEDIR)/" this_pkg ".extract" > DGEN_DEPS_POST;
>> + print "$(STATEDIR)/" this_pkg ".update: "
>> "$(STATEDIR)/" this_pkg ".extract" > DGEN_DEPS_POST;
>> + print "$(STATEDIR)/" this_pkg ".prepare: "
>> "$(STATEDIR)/" this_pkg ".update" > DGEN_DEPS_POST;
>
> The indention is wrong here. Make sure it's properly aligned with
> tabstop=8
What are exactly the rules for shell scripts? There seems to be an
indentation of 4 chars, but when are spaces required and when tabs?
>> diff --git a/scripts/lib/ptxd_make_world_patchin.sh
>> b/scripts/lib/ptxd_make_world_patchin.sh
>> index 57f7a61..a59abb9 100644
>> --- a/scripts/lib/ptxd_make_world_patchin.sh
>> +++ b/scripts/lib/ptxd_make_world_patchin.sh
>> @@ -28,6 +28,8 @@ ptxd_make_world_patchin_apply_init()
>> #
>> local path="${PTXDIST_PATH_PATCHES//://${pkg_pkg}/generic } \
>> ${PTXDIST_PATH_PATCHES//://${pkg_pkg} }"
>> + local pkg_patch_tool_git_ok=no
>> + local pkg_label="${pkg_stamp%%.*}"
>
> if this is not defined then a call to ptxd_make_world_init is missing
> somewhere.
It is not defined, so I have to check what is missing.
>> + else
>> + for each_pkg in ${PTXCONF_SETUP_PATCHIN_GIT_PKG_LIST}; do
>> + if [ "${each_pkg}" = "${pkg_label}" ]; then
>> + pkg_patch_tool_git_ok=yes
>> + break
>> + fi
>> + done
>
> if [[ " ${PTXCONF_SETUP_PATCHIN_GIT_PKG_LIST} " == *" ${pkg_label} "* ]]; then
> pkg_patch_tool_git_ok=yes
> fi
>
> or something like that
OK. It can always be done smarter ;-)
>> + fi
>> + fi
>> + if [ "${pkg_patch_tool_git_ok}" = "yes" ] && which git > /dev/null
>> 2>&1; then
>
> not your code but, but i think the check for git should be in bin/ptxdist:
> setup_config() and bailout if it's missing. This is a local user setting
> and should be off if git is not there.
OK
>> pkg_patch_tool=git
>> elif which quilt > /dev/null 2>&1; then
>> pkg_patch_tool=quilt
>> @@ -71,6 +85,69 @@ export -f ptxd_make_world_patchin_apply_init
>>
>>
>> #
>> +# Create a gignore file in the git-tree if it does not exist yet
>> +#
>> +ptxd_make_world_patchin_create_gitignore()
>> +{
>> + [ ! -f ${pkg_patchin_dir}/.gitignore ] || return
>> + cat > ${pkg_patchin_dir}/.gitignore << EOF
> [...]
>> +EOF
>
> don't inline it. Use files.
OK
> use ptxd_get_path to look for a file (e.g. pkg-gitignore) in
> ${pkg_patch_dir} and PTXDIST_PATH_PATCHES. This way you can put a special
> gitignore with the patches if necessary.
>
> Also: always adding .ptxdist/ to .git/info/exclude would be nice (what's
> the 'correct' way to edit this file?).
I will check
>> ptxd_make_world_patchin_apply_git_init()
>> @@ -81,6 +158,7 @@ ptxd_make_world_patchin_apply_git_init()
>> # is already git repo?
>> if [ "${git_dir}" != ".git" ]; then
>> echo "patchin: git: initializing repository"
>> + ptxd_make_world_patchin_create_gitignore
>> git init -q &&
>> git add -f . &&
>> git commit -q -m "initial commit"
>> --author="ptxdist-${PTXDIST_VERSION_FULL} <[email protected]>" &&
>
> This has the issue, that the commit will always be different (timestamps
> etc.). I've been playing with git-write-tree and git-commit-tree and I
> think it's possible to reproduce the exact same commit. Do you think that's
> useful?
How? Git always puts extra information in the patch-header that quilt
does not do.
I thought git can only reproduce the same patch if it has generated it
in the first place.
Further, we have in our own patchsets a subdirectory structure that
quilt understands, but git does not.
(We have per package a directory structure defined where a split up
has been made about local patches, published, and mainline accepted
patches)
>> + grep -v '/staged/' | sort | xargs cat | md5sum | cut
>> -d' ' -f1)
>> + git tag -f -m "md5:${patches_current_md5}" patches-current
>> }
>> export -f ptxd_make_world_patchin_apply_git
>>
>
> all the ptxd_make_world_update_* stuff should be in
> scripts/lib/ptxd_make_world_update.sh
OK
>> +#
>> +# returns nonzero in case the <PKG>.update needs to be updated, zero if not
>> +ptxd_make_world_update()
>> +{
>> + local pkg_update_touch_stage=0
>> + local pkg_patchin_dir=${pkg_deprecated_patchin_dir:-${pkg_dir}}
>
> all stuff with *_deprecated_* should be in ptxd_make_world_init_compat.
> Make a cleanup patch for that first please.
???
>> + pushd "${pkg_patchin_dir}" > /dev/null
>> +
>> + ptxd_make_world_update_cleanup
>> + ptxd_make_world_update_export_staged
>> +
>> + #
>> + # When the patches directory contents have changed, we need to rebuild
>> the
>> + # entire git tree and move our current work on top of it.
>> + #
>> + patches_current_md5=$(find ${pkg_patchin_dir}/.ptxdist/patches/ -type f
>> | \
>> + grep -v '/staged/' | sort | xargs cat | md5sum | cut -d' '
>> -f1)
>> + patches_stored_md5=$(git show -s --pretty=format: patches-current |
>> grep 'md5' | cut -d':' -f2)
>> + if [ "${patches_current_md5}" != "${patches_stored_md5}" ]; then
>> + echo "--> Patch series has changed! Rebuilding the git tree"
>
> I'm not sure if I'm reading this correctly: you're only checking if the
> external patches (from ptxdist) changed, not if someone committed something
No,we also check the patches in the projects own directory, not just ptxdist.
> locally, right? We may not need to rebase, but certainly rebuild.
> hmmm, but not reconfigure. Maybe this:
We do not automatically reconfigure, since that would practically
result in a complete rebuild of the package.
Imagine what build-time it would take if we would reconfigure the kernel...
> in the stage I check stage I proposed:
> - if the patches changed -> rm $(STATEDIR)/<pkg>.update
> - if the tree changed -> rm $(STATEDIR)/<pkg>.compile
Well, when local commits are made, these commits will get exported to
the patches/staged directory, and only if the commits changed, that
would result in to a recompile, NOT a reconfigure. Local uncommitted
changes would NOT result in a recompile, since detecting local
uncommitted changes can be time consuming, which slows down the build
too much.
>> + # Check for uncommitted changes
>> + if git update-index --refresh --unmerged | read dummy; then
>> + ptxd_bailout "Working tree dirty, refusing to update the git
>> tree"
>> + fi
>> + if git diff-index --name-only HEAD | read dummy; then
>> + ptxd_bailout "Working tree dirty, refusing(2) to update the
>> git tree"
>> + fi
>> + # Now do the rebuild of the git-tree and rebase
>> + (git checkout -b temp_old patches-current &&
>> + git checkout -b temp_new base &&
>> + ptxd_make_world_patchin_apply_git &&
>> + git rebase --onto temp_new temp_old master &&
>> + git branch -D temp_old &&
>> + git branch -d temp_new) || \
>> + ptxd_bailout "Failed to rebuild the git tree in ${pkg_patchin_dir}"
>
> hmmm, this allows me to add new patches on top of the existing ones. What's
> the workflow for changing existing patches? That's what I do most of the
> time. Fixing comments, signed-offs, updating the patches for a newer
> version, etc.
For normal incremental development you should not change history. But
whenever all stuff is finalised, you can 'rebase -i' your tree
whenever you wish and export the patches with the older git-alias and
export the new set to the patches directory.
ptxdist will on a new incremental build restart the reconfigure, and
guess what, a new tree is being generated, and even duplicate patches
are removed by git...
Maybe you can do a test run of this patch and see how it feels. Maybe
that would result in new ideas from your end to improve it even
further.
>> + pkg_update_touch_stage=1
>> + fi
>> + popd > /dev/null
>
> What's this for? We back in make after this and the current dir is reseted.
copy-paste from other functions... it is done at more places, I
thought it was clean to keep it balanced.
Thanks for the review comments.
You have some fundamental remarks that might take me some time to fix,
so please do not expect an update of this patch tomorrow... ;-))
Kind regards,
Remy
--
ptxdist mailing list
[email protected]
