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