Hi Daniel,
Dne 18.08.2024 (ned) ob 13:05 +0200 je Daniel Gröber napisal(a):
> 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.
>
Thanks for the review and for 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".
>
I agree, i'll change that text.
> 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.
>
I generally agree. I look at generating/cleaning test repos as the whole
replacement for the currently commited binary repos. And both generating and
cleaning should have been done upstream at the end, i believe. Another thing is
moving/restoring commited binary repos while they still exist in upstream, which
i also prepared through patches. Do you see mixing generating/cleaning and
moving/restoring as juggling upstream and downstream changes?
> > 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 very strange, i found no such behavior regarding speed or errors below.
I made update/upgrade of my schroot as well. I only get errors, when i call
tests without our patches.
Can you provide more info on how to reproduce this?
>
> 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" ?
>
This ensures that TMPREPO is set to absolute path even if $1 argument is a
relative path. Upstream uses that construct several times.
> 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.
Thanks for pointing to 'shellcheck'. I wasn't aware of it and i'll try to fix
all added scripts.
>
> > > 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.
>
As i already mentioned, i see generation and cleanup of test repos as integral
parts of the commited binary test repos replacement to be done upstream.
> 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.
>
Let's hope upstream does right that with a little help of our patches.
> 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.
>
Maybe i got your point here. So, dealing with original commited test repos would
have been handled in d/rules, while generation/cleanup of new test repos is
still done in patches for upstream without original commited test repos.
I'll consider that. Do i understand correctly your suggestion?
> > I had problems calling 'debuild -uc -us; debuid -- clean', because cleaning
> > gets
> > called upon the unpatched project.
>
> I'm not sure what you mean?
I meant that 'debuild -- clean' after running 'debuild -uc -us' calls 'make
clean' in the unpatched project.
> 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 :)
>
I'll try to prepare something and hopefully things will be clearer.
> 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.
>
exactly:)
regards, Samo
