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. - tests fail in a git checkout (but not during clean sbuild it seems) - 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 :) 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. 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. 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) Upstream's suggestion to put it in test/setup is even better though so that's the way to go. > 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. Thanks, --Daniel
signature.asc
Description: PGP signature
