Hi Daniel,
I prepared a new forced push to my git-repo, where i tried to address all the
comments and suggestions from your latest review. I am not sure if my changes to
d/rules are exactly what you'd expect, but let's see.
best regards, Samo
p.s.
I left my previous reply to your review below just for reference.
Dne 22.08.2024 (čet) ob 22:34 +0200 je Samo Pogačnik napisal(a):
> 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
>
