Package: git-subrepo Dear Maintainer, Eeer I mean, Hi Samo,
since our ITP #979188 got fixed by being ACCEPTED into unstable we should
start using a new email sink to CC our conversation to since the BTS stops
accepting emails for "archived" (i.e. closed) issues after a while so I'm
opening a new bug for the testsuite.
On Thu, Aug 15, 2024 at 09:22:14PM +0200, Samo Pogačnik wrote:
> > On the Debian git side you just replace the d/patch/* files and commit that
> > change.
>
> I've replaced patches 0004 and 0005, re-enabled tests and pushed changes
> to my salsa repo.
Nit: The commit subjects could have been more useful. "Replace patch ..."
is not as useful as describing the actual change implemented. I would have
committed both patches at once as something like "Allow running tests
outside Makefile".
You mixed a change to 'make clean' into the 0005 commit as well. Ideally
that should have been a seperate commit, but since juggling upstream and
downstream changes at the same time can be a bit unweildy I can accept that
sort of thing, but I'd still remind you it's discouraged in general.
> Rewritten patches deal with tests so that running tests does not require
> running them only via Makefile. Also package cleaning was extended to
> remove generated test repos and to restore the original 'binary' test
> repos, while they still exist.
Running the tests seems excessively slow now. Each test/* line takes 15+ish
seconds now. That doesn't seem right. The build/tests succeed via sbuild
but I'm seeing test failures building right inside my unstable schroots
(via dpkg-buildpackage/debuild) and on my bookworm host:
This is in a freshly updated unstable schroot:
````
$ dpkg-buildpackage -uc -us
[...]
test/branch-all.t .................. ok
test/branch-rev-list-one-path.t .... Died at line 23 in main of
test/branch-rev-list-one-path.t
test/branch-rev-list-one-path.t .... No subtests run
test/branch-rev-list.t ............. Died at line 26 in main of
test/branch-rev-list.t
test/branch-rev-list.t ............. No subtests run
test/branch.t ...................... ok
test/clean.t ....................... ok
test/clone-annotated-tag.t ......... ok
test/clone.t ....................... ok
test/compile.t ..................... ok
test/config.t ...................... ok
test/encode.t ...................... ok
test/error.t ....................... ok
test/fetch.t ....................... ok
test/gitignore.t ................... ok
test/init.t ........................ ok
test/issue29.t ..................... ok
test/issue95.t ..................... 1/? # Failed test at test/issue95.t line
94.
# got: 'The "git merge" command failed:
#
# hint: Diverging branches can't be fast-forwarded, you need to either:
# hint:
# hint: git merge --no-ff
# hint:
# hint: or:
# hint:
# hint: git rebase
# hint:
# hint: Disable this message with "git config advice.diverging false"
# fatal: Not possible to fast-forward, aborting.
#
# You will need to finish the pull by hand. A new working tree has been
# created at .git/tmp/subrepo/sub so that you can resolve the conflicts
# shown in the output above.
#
# This is the common conflict resolution workflow:
#
# 1. cd .git/tmp/subrepo/sub
# 2. Resolve the conflicts (see "git status").
# 3. "git add" the resolved files.
# 4. git commit
# 5. If there are more conflicts, restart at step 2.
# 6. cd /home/dxld/share/dev/deb/pkg/git-subrepo/test/tmp/host
# 7. git subrepo commit sub
# See "git help merge" for details.
#
# Alternatively, you can abort the pull and reset back to where you started:
#
# 1. git subrepo clean sub
#
# See "git help subrepo" for more help.'
# expected: 'Subrepo 'sub' pulled from '../sub' (main).'
test/issue95.t ..................... Failed 1/1 subtests
test/issue96.t ..................... 1/? # Failed test at test/issue96.t line
86.
# got: 'The "git merge" command failed:
#
# hint: Diverging branches can't be fast-forwarded, you need to either:
# hint:
# hint: git merge --no-ff
# hint:
# hint: or:
# hint:
# hint: git rebase
# hint:
# hint: Disable this message with "git config advice.diverging false"
# fatal: Not possible to fast-forward, aborting.
#
# You will need to finish the pull by hand. A new working tree has been
# created at .git/tmp/subrepo/sub so that you can resolve the conflicts
# shown in the output above.
#
# This is the common conflict resolution workflow:
#
# 1. cd .git/tmp/subrepo/sub
# 2. Resolve the conflicts (see "git status").
# 3. "git add" the resolved files.
# 4. git commit
# 5. If there are more conflicts, restart at step 2.
# 6. cd /home/dxld/share/dev/deb/pkg/git-subrepo/test/tmp/host
# 7. git subrepo commit sub
# See "git help merge" for details.
#
# Alternatively, you can abort the pull and reset back to where you started:
#
# 1. git subrepo clean sub
#
# See "git help subrepo" for more help.'
# expected: 'Subrepo 'sub' pulled from '../sub' (main).'
git-subrepo: There are new changes upstream, you need to pull first.
# Failed test at test/issue96.t line 94.
# got: ''
# expected: 'Subrepo 'sub' pushed to '../sub' (main).'
test/issue96.t ..................... Failed 2/2 subtests
test/pull-all.t .................... ok
test/pull-merge.t .................. 3/? # Failed test 'The readme file in
the mainrepo is merged'
# at test/pull-merge.t line 63.
# got: 'new file Bar2
# bar/Bar2'
# expected: 'Merged Bar2'
# Failed test 'subrepo pull should have merge message'
# at test/pull-merge.t line 70.
# Got: 'modified file: bar/Bar2'
# Failed test '.gitrepo commit is correct'
# at test/setup line 182.
# got: 'eb8738f1ac8b9d66f908478f3a7c5dc961a18717'
# expected: '8a5e72212d1c84757308d75b22c1a4fd20dce58d'
Died at line 85 in main of test/pull-merge.t
# Tests were run but no plan was declared.
test/pull-merge.t .................. Failed 3/8 subtests
test/pull-message.t ................ ok
test/pull-new-branch.t ............. ok
test/pull-ours.t ................... 1/? Died at line 73 in main of
test/pull-ours.t
# Tests were run but no plan was declared.
test/pull-ours.t ................... All 4 subtests passed
test/pull-theirs.t ................. 1/? # Failed test 'The readme file in
the mainrepo is theirs'
# at test/pull-theirs.t line 57.
--- /tmp/want-1197305 2024-08-18 10:50:46.449806096 +0000
+++ /tmp/got-1197305 2024-08-18 10:50:46.449806096 +0000
@@ -1,2 +1,2 @@
new file Bar2
-Bar2
+bar/Bar2
2 4 19 /tmp/want-1197305
2 4 23 /tmp/got-1197305
4 8 42 total
Died at line 65 in main of test/pull-theirs.t
# Tests were run but no plan was declared.
test/pull-theirs.t ................. Failed 1/3 subtests
test/pull-twice.t .................. Died at line 24 in main of
test/pull-twice.t
test/pull-twice.t .................. No subtests run
test/pull-worktree.t ............... ok
test/pull.t ........................ ok
test/push-after-init.t ............. ok
test/push-after-push-no-changes.t .. ok
test/push-force.t .................. ok
test/push-new-branch.t ............. ok
test/push-no-changes.t ............. ok
test/push-squash.t ................. ok
test/push.t ........................ git-subrepo: There are new changes
upstream, you need to pull first.
test/push.t ........................ No subtests run
test/reclone.t ..................... ok
test/shellcheck.t .................. skipped: The 'shellcheck' utility is not
installed
test/status.t ...................... ok
test/submodule.t ................... ok
test/zsh.t ......................... skipped: The 'docker' utility is not
installed
Test Summary Report
-------------------
test/branch-rev-list-one-path.t (Wstat: 0 Tests: 0 Failed: 0)
Parse errors: No plan found in TAP output
test/branch-rev-list.t (Wstat: 0 Tests: 0 Failed: 0)
Parse errors: No plan found in TAP output
test/issue95.t (Wstat: 0 Tests: 1 Failed: 1)
Failed test: 1
test/issue96.t (Wstat: 0 Tests: 2 Failed: 2)
Failed tests: 1-2
test/pull-merge.t (Wstat: 0 Tests: 8 Failed: 3)
Failed tests: 5-7
Parse errors: No plan found in TAP output
test/pull-ours.t (Wstat: 0 Tests: 4 Failed: 0)
Parse errors: No plan found in TAP output
test/pull-theirs.t (Wstat: 0 Tests: 3 Failed: 1)
Failed test: 3
Parse errors: No plan found in TAP output
test/pull-twice.t (Wstat: 0 Tests: 0 Failed: 0)
Parse errors: No plan found in TAP output
test/push.t (Wstat: 0 Tests: 0 Failed: 0)
Parse errors: No plan found in TAP output
Files=38, Tests=293, 58 wallclock secs ( 0.12 usr 0.06 sys + 36.58 cusr 25.84
csys = 62.60 CPU)
Result: FAIL
make[2]: *** [Makefile:52: test] Error 1
make[2]: Leaving directory '/home/dxld/share/dev/deb/pkg/git-subrepo'
make[1]: *** [debian/rules:13: override_dh_auto_test] Error 2
make[1]: Leaving directory '/home/dxld/share/dev/deb/pkg/git-subrepo'
make: *** [debian/rules:8: binary] Error 2
````
Looking over the patches:
> --- /dev/null
> +++ b/test/genbar
> @@ -0,0 +1,32 @@
> +#!/bin/bash
> +set -xe
> +
> +if [ -z "${1}" ]; then
> + echo "${BASH_SOURCE[0]}: Single argument required (common test repos
> pa> th)"
> + exit 1
> +fi
> +
> +REPO="bar"
> +NAME="Bar"
> +TARGET="${1}/$REPO"
> +TMPREPO="$(cd ${1} && pwd )/tmp/$REPO"
That TMPREPO thing looks a bit fishy to me. What's the thinking here? Why
not just ="$1/tmp/$REPO" ?
Also you don't handle the [ $# -eq 0 ] case where $1 is empty. I recommend
`set -xeu` if you don't want to think about that edgecase here. Still, $1
should at least be a named variable. The code is more self documenting that
way.
Also: you don't quote $1 inside $(). Please checkout shellcheck.1 it finds
this sort of thing easily:
$ shellcheck test/genfoo
In test/genfoo line 12:
TMPREPO="$(cd ${1} && pwd )/tmp/$REPO"
^--^ SC2086 (info): Double quote to prevent globbing and word
splitting.
Did you mean:
TMPREPO="$(cd "${1}" && pwd )/tmp/$REPO"
In test/genfoo line 14:
rm -rf $TMPREPO
^------^ SC2086 (info): Double quote to prevent globbing and word
splitting.
Did you mean:
rm -rf "$TMPREPO"
In test/genfoo line 15:
mkdir -p $TMPREPO
^------^ SC2086 (info): Double quote to prevent globbing and word
splitting.
Did you mean:
mkdir -p "$TMPREPO"
In test/genfoo line 16:
mv -f $TARGET ${TMPREPO}_orig
^-----^ SC2086 (info): Double quote to prevent globbing and word
splitting.
^--------^ SC2086 (info): Double quote to prevent globbing
and word splitting.
Did you mean:
mv -f "$TARGET" "${TMPREPO}"_orig
In test/genfoo line 17:
cd $TMPREPO
^------^ SC2086 (info): Double quote to prevent globbing and word
splitting.
Did you mean:
cd "$TMPREPO"
In test/genfoo line 26:
mkdir -p $1
^-- SC2086 (info): Double quote to prevent globbing and word
splitting.
Did you mean:
mkdir -p "$1"
In test/genfoo line 27:
mv $TMPREPO/.git $TARGET
^------^ SC2086 (info): Double quote to prevent globbing and word
splitting.
^-----^ SC2086 (info): Double quote to prevent globbing
and word splitting.
Did you mean:
mv "$TMPREPO"/.git "$TARGET"
These should be fixed across the new scripts ideally.
> > I do that with `debuild -uc -us; debuild -- clean`, possibly inside a sid
> > schroot (not necessary for git-subrepo iirc).
>
> By running 'debuild -uc -us -tc' cleanup is automatically executed after the
> build upon patched project and the project is restored by its patche wisdom to
> its pre-build state.
Right, from the outside that looks good, but I think you're mixing distro
specific fixes into your upstream changes here.
With an upstream first mindset the instinct should be to remove the old
binary test repos and fully replace them by the new scipts instead of
scripting around them.
From a Debian perspective doing the move+restore thin is perfectly fine,
but then it ought to be done in d/rules where it's more visible to fellow
debianites.
> I had problems calling 'debuild -uc -us; debuid -- clean', because cleaning
> gets
> called upon the unpatched project.
I'm not sure what you mean? AFAIK debuild should call `dpkg-source
--before-build .` before calling `d/rules clean` and leave the patches
applied after the build. There is a config toggle somewhere to override the
latter IIRC but I don't think we have that.
If I manually drop the patches before calling clean -- yeah it doesn't
clean properly. That's another reason to put this Debian specific cleaning
logic in d/rules where it's always active. Alternatively just remove the
old test repos in the patch :)
Dropping down one layer of abstraction to make sure debuild doesn't do anything
weird:
````
$ dpkg-buildpackage -uc -us
dpkg-buildpackage: info: source package git-subrepo
dpkg-buildpackage: info: source version 0.4.6-1
dpkg-buildpackage: info: source distribution unstable
dpkg-buildpackage: info: source changed by Samo Pogačnik <samo_pogac...@t-2.net>
dpkg-buildpackage: info: host architecture amd64
dpkg-source --before-build .
dpkg-source: info: using patch list from debian/patches/series
dpkg-source: info: applying 0001-Removed-hashbangs-from-sourced-only-files.patch
dpkg-source: info: applying
0002-Removed-executable-permission-on-sourced-only-files.patch
dpkg-source: info: applying
0003-Fixed-bash-completion-integration-with-git.patch
dpkg-source: info: applying 0004-Fixed-make-test-for-debian-build.patch
dpkg-source: info: applying 0005-Generate-test-git-repos-foo-bar-init.patch
debian/rules clean
[...]
````
seems fine.
--Daniel
signature.asc
Description: PGP signature
