Hi,
On Fri, Nov 12, 2010 at 09:01:24PM +0100, Remy Bohmer wrote:
> >> @@ -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 ;-)
Agreed. I just wanted to mention it.
> > 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...
So you basically want to apply two patch series, right? I can see the use
case but it sounds really scary.
> >> +# $(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.
The handling of deprecated stuff is already a mess. No need to add to 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.
I took your idea to only update a stage if something really changed to only
calculate new md5sums for the patches for all packages. It's still not fast
enough. In the worst case (e.g. a newly installed ptxdist) all md5sums must
be compared. This makes "ptxdist go" take twice as long if there is
nothing to do :-(. I appended the patch at the end. Maybe you can mix it
with only doing this for selected packages.
> >> 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?
Indent level is 4. Replace 8 with one <tab>. This is some strange emacs
indention style.
> >> 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.
That's because you don't have "ptxd_make_world_init || return" at the
beginning of ptxd_make_world_update.
> >> + 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.
Well you only need From, Data, Subject headers. quilt won't touch these. If
they exist and you apply the same patches to the same tree, then the
resulting commit hashes should be the same.
The problem right now is that the initial commit is always different
because the data of the first commit.
> (We have per package a directory structure defined where a split up
> has been made about local patches, published, and mainline accepted
> patches)
Interesting idea.
> >> + 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.
>
> ???
you copied "local pkg_patchin_dir=..." from ptxd_make_world_patchin, right?
It should not be defined there. Stuff like that is handled in
ptxd_make_world_init_compat.
> >> + 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.
That's what I meant.
> > 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
Are you sure? I think the "grep -v '/staged/'" removes the staged patches
from the list of patches.
> changes would NOT result in a recompile, since detecting local
> uncommitted changes can be time consuming, which slows down the build
> too much.
Ok.
> >> + # 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.
I think the main issue will be the work flow. Right now I'm exporting the
patches quite often and before they are complete. Just to make sure my work
is saved somewhere. The staged directory takes care of the now, right?
Maybe we could reimport it as a branch in case we had to clean the packages
for some other reason.
> >> + 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.
It's only necessary if you do something afterwards like patchin does.
Otherwise use 'cd' instead of pushd and drop the popd.
> 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... ;-))
Of course. This will take some time to get ready but I'm looking forward to
work with it.
Michael
diff --git a/rules/post/ptxd_make_md5_check.make
b/rules/post/ptxd_make_md5_check.make
new file mode 100644
index 0000000..bbadf11
--- /dev/null
+++ b/rules/post/ptxd_make_md5_check.make
@@ -0,0 +1,25 @@
+# -*-makefile-*-
+#
+# Copyright (C) 2010,Michael Olbrich <[email protected]>
+#
+# See CREDITS for details about who has contributed to this project.
+#
+# For further information about the PTXdist project and license conditions
+# see the README file.
+#
+
+md5_write = \
+ ptxd_make_md5_write $@ $^
+
+md5_check = \
+ ptxd_make_md5_check $@ $(1)
+
+$(STATEDIR)/%.patches.md5:
+ @$(call targetinfo)
+ @$(call md5_write)
+
+$(STATEDIR)/%.patches: $(STATEDIR)/%.patches.md5
+ @$(call md5_check, [email protected])
+
+
+# vim: syntax=make
diff --git a/rules/pre/autogen.make b/rules/pre/autogen.make
index 584d140..283798a 100644
--- a/rules/pre/autogen.make
+++ b/rules/pre/autogen.make
@@ -12,6 +12,10 @@ autogen_dep = $(strip $(wildcard
\
$(foreach dir,$(subst :,/$(strip
$(1))$(space),$(PTXDIST_PATH_PATCHES)),\
$(dir)/generic/autogen.sh $(dir)/autogen.sh)))
+pkg_patchdir = $(if $(1),$(strip $(wildcard
\
+ $(foreach dir,$(subst :,/$(strip
$(1))$(space),$(PTXDIST_PATH_PATCHES)),\
+ $(dir)/generic $(dir)))))
+
$(STATEDIR)/autogen-tools:
@$(call targetinfo)
@$(call touch)
diff --git a/scripts/lib/ptxd_lib_dgen.awk b/scripts/lib/ptxd_lib_dgen.awk
index d446412..c7f3f5d 100644
--- a/scripts/lib/ptxd_lib_dgen.awk
+++ b/scripts/lib/ptxd_lib_dgen.awk
@@ -285,6 +285,7 @@ function import_PKG(this_PKG, this_pkg) {
print this_PKG "_DEVPKG = cross-" this_devpkg >
DGEN_DEPS_PRE;
}
}
+ print this_PKG "_PATCHDIR = $(call pkg_patchdir,$(" this_PKG "))">
DGEN_DEPS_PRE;
}
END {
@@ -299,6 +300,8 @@ END {
#
# default deps
#
+ print "$(STATEDIR)/" this_pkg ".patches.md5: $(if $(" this_PKG
"_PATCHDIR),$(" this_PKG "_PATCHDIR)/*)"> DGEN_DEPS_POST;
+ print "$(STATEDIR)/" this_pkg ".extract: "
"$(STATEDIR)/" this_pkg ".patches" > DGEN_DEPS_POST;
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 ".tags: "
"$(STATEDIR)/" this_pkg ".prepare" > DGEN_DEPS_POST;
diff --git a/scripts/lib/ptxd_make_md5_check.sh
b/scripts/lib/ptxd_make_md5_check.sh
new file mode 100644
index 0000000..bbccd7d
--- /dev/null
+++ b/scripts/lib/ptxd_make_md5_check.sh
@@ -0,0 +1,38 @@
+#!/bin/bash
+#
+# Copyright (C) 2010 by Michael Olbrich <[email protected]>
+#
+# See CREDITS for details about who has contributed to this project.
+#
+# For further information about the PTXdist project and license conditions
+# see the README file.
+#
+
+ptxd_make_md5_write() {
+ stamp="${1}"
+ shift
+ if [ $# -eq 0 ]; then
+ echo | md5sum > "${stamp}"
+ else
+ cat "$@" | md5sum > "${stamp}"
+ fi
+}
+export -f ptxd_make_md5_write
+
+ptxd_make_md5_check() {
+ local old_stamp new_stamp old_md5 new_md5
+
+ old_stamp="${1}"
+ new_stamp="${2}"
+
+ old_md5=$(cat "${old_stamp}" 2> /dev/null)
+ new_md5=$(cat "${new_stamp}" 2> /dev/null)
+
+ if [ "${old_md5}" = "${new_md5}" ]; then
+ return
+ fi
+ cp "${new_stamp}" "${old_stamp}"
+ echo "md5 changed updating target ${old_stamp}"
+}
+export -f ptxd_make_md5_check
+
--
1.7.2.3
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
ptxdist mailing list
[email protected]
