Hi Daniel,

Thanks for the reply and the initial upload.

Dne 02.08.2024 (pet) ob 17:10 +0200 je Daniel Gröber napisal(a):
> Hi Samo,
> 
> On Mon, Jun 24, 2024 at 04:53:01PM +0200, Samo Pogačnik wrote:
> > i prepared another candidate for the 0.4.6-1 release (
> > 
https://salsa.debian.org/spog/git-subrepo/-/commit/f96eeedd0e96b6f2bbcc8c013909de5d5325cafe
> > ), hoping it ticks all the boxes and more :)
> 
> Excellent!
> 
> I did run into some more issues during build/testing unfortunately. Some
> beurocratic, some related to tests:
> 
>  - you are still missing from d/copyright :)
> 
>  - d/changelog should not contain versions that were never in Debian.
>    consequently everything that isn't the "Intial release" entry also doesn't
>    make sense. Not sure why I didn't catch that before.
> 
thanks for fixing both points above

>  - tests fail in a git checkout (but not during clean sbuild it seems)
> 
I'll have to re-check what is going on here.

>  - d/rules clean doesn't cleanup after tests
> 
> The package looks good sans tests so I'll upload that to NEW so we can keep
> working on the tests in the meantime. Remeber to pull my changes from
> debian/git-subrepo before you continue working :)
> 
Yes, that is the first thing i need to do - i am slowly catching reality:)

> For the cleanup requirement see
> 
https://www.debian.org/doc/debian-policy/ch-source.html#main-building-script-debian-rules
> 
> > clean (required)
> > 
> >    This must undo any effects that the build and binary targets may have
> >    had, except that it should leave alone any output files created in the
> >    parent directory by a run of a binary target.
> 
> Right now I have to `git reset --hard HEAD; git clean -xfd` to get rid of
> all the build byproducts.
> 
> This is one of the more onerous requirements of Debian policy rooted in a
> world that didn't have git yet. It's important because some people still
> work on packages without git and they need a way to reset the build tree.
> 
> Looking at what remains after `quilt pop -a` I see untracked files
> test/repo/* which you create with your 0005 patch. Really that should
> probably be in test/tmp which upstream has already setup to be removed by
> their clean target.
> 
Acknowledged. I'll learn about this.

> Other than that I see changes to upstream files:
> 
> modified   test/repo/bar/config
> @@ -2,3 +2,7 @@
>       repositoryformatversion = 0
>       filemode = true
>       bare = true
> +     logallrefupdates = true
> +[user]
> +     name = BarUser
> +     email = bar@bar
> deleted    test/repo/bar/objects/94/c86ffc745232d89f78c6f895e11e71272518db
> deleted    test/repo/bar/objects/f6/2a8ff3feadf39b0a98f1a86ec6d1eb33858ee9
> modified   test/repo/bar/refs/heads/master
> @@ -1 +1 @@
> -94c86ffc745232d89f78c6f895e11e71272518db
> +e2aebf65b96f0be7595bffed8ff42cc69334f150
> modified   test/repo/bar/refs/tags/A
> @@ -1 +1 @@
> -f62a8ff3feadf39b0a98f1a86ec6d1eb33858ee9
> +f8206189232afa81e999b1b3aeb524fc4d9bae46
> modified   test/repo/foo/config
> @@ -2,3 +2,7 @@
>       repositoryformatversion = 0
>       filemode = true
>       bare = true
> +     logallrefupdates = true
> +[user]
> +     name = FooUser
> +     email = foo@foo
> deleted    test/repo/foo/objects/e2/1291a1ad392a9d4c51dd9586804f1467b28afd
> modified   test/repo/foo/refs/heads/master
> @@ -1 +1 @@
> -e21291a1ad392a9d4c51dd9586804f1467b28afd
> +e51641c054bb9e0591a8a95d0789e10abb308de2
> modified   test/repo/init/config
> @@ -1,5 +1,8 @@
>  [core]
>       repositoryformatversion = 0
>       filemode = true
> -     bare = false
> +     bare = true
>       logallrefupdates = true
> +[user]
> +     name = InitUser
> +     email = init@init
> deleted    test/repo/init/objects/32/5180321750a21cd7a4e7ecda319e557a4f6a09
> deleted    test/repo/init/objects/3d/918c6901c02f43af5d31779dd5e1f9166aeb36
> deleted    test/repo/init/objects/58/931fc1bd559b59c41ea738fc7ad04f9ad01bd3
> deleted    test/repo/init/objects/94/7b3d714c38791e95ad6f928b48c98bb8708acd
> deleted    test/repo/init/objects/c3/ee8978c4c5d84c3b7d00ba8e5906933d027882
> modified   test/repo/init/refs/heads/master
> 
> In general you must not change upstream files during the Debian package
> build. Sometimes this is necessary, in such cases a save+restore approach
> is taken cf. dh_autoreconf. However this is not appropriate here.
> 
> 
Noted, and hopefully we are going to find a proper way to remove binary test
repos already upstream.

> Also, looking at 0005:
> 
> > diff --git a/Makefile b/Makefile
> > index f84b83d..1927073 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -47,6 +47,18 @@ help:
> >     @echo 'uninstall  Uninstall $(NAME)'
> >     @echo 'env        Show environment variables to set'
> >  
> > +.PHONY: genfoo
> > +genfoo:
> > +   test/genfoo test/repo
> > +
> > +.PHONY: genbar
> > +genbar:
> > +   test/genbar test/repo
> > +
> > +.PHONY: geninit
> > +geninit:
> > +   test/geninit test/repo
> > +
> >  .git:
> >     git init -b main .
> >     git config user.email "you@you"
> > @@ -57,6 +69,9 @@ help:
> >  
> >  .PHONY: test
> >  test: .git
> > +   make genfoo
> > +   make genbar
> > +   make geninit
> >     prove $(prove) $(test)
> 
> Recursive make calls are uncessary here, but you're also ignoring the
> arcane convention to call $(MAKE), see
> https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html.
> 
> Easy fix is to just add dependencies to the test target, like so:
>  
>  .PHONY: test
> -test: .git
> +test: .git genfoo genbar geninit
>       prove $(prove) $(test)
> 
yp, much nicer and thanks for the $(MAKE) call info:)

> Upstream's suggestion to put it in test/setup is even better though so
> that's the way to go.
> 
Yes, i've already acknowledged this in the upstream PR.

> > I made the PR upstream
> > (https://github.com/ingydotnet/git-subrepo/pull/623) and added
> > 'Forwarded' fields with PR to all five patches. I hope that making a
> > separate commit for adding 'Forwarded' field to patches is ok.
> 
> It's what I would have done :)
:)

> 
> > The 'git-subrepo.d' subdir has been removed upon 'make install' and
> > additional
> > Makefile adjustment done according to your suggestions.
> > 
> > Regarding internal test suite, things weren't that trivial, as the
> > implementation requires that the project is a git worktree. However debian
> > builds from a non-git tarball. I fixed that by git-initializing the project
> > on
> > the fly, when needed as well as providing local git configuration for all
> > repos
> > involved in tests. Another thing regarding tests was that they rely upon
> > english
> > output, so tests failed in reprotest, when executed in a French environment
> > for
> > example. By setting fixed locale during each test being run, it all went
> > well.
> 
> Right that all sounds good.
> 
> I did consider git-init'ing the whole package build directory when I was
> looking at this but doing that (and complying with policy) would be fiddly
> beyond belief so you made the right call.
:)

> 
> > I also went a step further regarding tests. Three test repos being used by
> > several tests were committed upstream as binary bare git repos, which i
> > really
> > do not like. So i prepared a few scripts, which generate the same repos with
> > the
> > same history upon each test suite startup.
> 
> IMO git-subrepo's upstream testsuite made some bad decisions here and I
> hope we can convince them this way is better.
> 
> I hope you are planning to address admorgan's comments on the upstream PR
> since they seem reasonable and the idea when sending patches upstream
> really is that we get to drop them on the next release to keep the Debian
> package from continuing to diverge.
> 
Yes, i am planning to address the comments, however upstream needs to be
compatible with git on Windows, which may result in some permanent debian
patches.

Thanks again, Samo

Reply via email to