Hi Samo, On Fri, Jun 14, 2024 at 07:15:40PM +0200, Samo Pogačnik wrote: > Thanks for the review. I'll do my best to include as much as i know how > to into another 'release candidate', however it may take me a while.
Thanks for working on this :) Recent discussion on d-devel makes me think git-subtree/subrepo have a good chance of becoming more popular as we educate upstreams about how to properly avoid future xz snafu. So you're doing really important work here <3 > Dne 10.06.2024 (pon) ob 22:26 +0200 je Daniel Gröber napisal(a): > > In general you're missing the Debian patch metadata, see [DEP-3]. I hate > > this archaic stuff I just mention it for completness. If you use gbp-pq for > > generating the patches you can mostly ignore. I do like to use the > > Forwarded field to note the upstream PR for the patch tho. > > > > [DEP-3]: https://dep-team.pages.debian.net/deps/dep3/ > > > > Next time I see the patches I want to see a Forwarded field for each -- one > > PR for all of them pleas, not spam upstream :) > > > I am a bit confused. I'm not sure i want to present patches upstream while you > (at least) have not yet accepted them. Otherwise i'd have to revise my > upstream > proposal (and spam upstream) every time you find another thing i missed about > the debian policy (which i probably will for some time). How do you see this? That's not spamming upstream so much as just doing upstream development :) There's nothing wrong with pushing revisions in that case. If you insist you can send me the patches/branch for review first, but I don't see a need for that. I was just trying to be clear about the fact that these patches should be well motivated and single-purpose so upstream can understand them. If it happens that upstream and Debian policy disagree there's no problem with sending upstream the patches they'll take and just add the rest on top in the Debian package patch stack. I do think we should try to always push patches upstream first to keep them informed about what we're doing. The "spam" comment was really just me making sure you're aware you're not expected to split each patch into it's own PR. Don't worry about that too much. Looking at the upstream activity I fear us being ignored may the worst that'll happen. My hunch is that we may end up having to take over as git-subrepo upstream. Only one way to know (send a PR) and in any case we'd want clean commits :) One workflow note: whatever you do you should make sure not to have Gbp-* fields in the commits you send upstream. Getting the Debian tooling to cooperate here can be a bit tricky. Give it a go and let me know if you need help. Overall what I'd try to do is commit the upstream changes[1] on top of our debian/sid branch, build/test using the Debian tooling and once things are working rebase onto the plain upstream branch then push that as a PR. The problem you're going to run into is dpkg-source complaining about unrepresented upstream changes. IIRC you can bypass that problem by only doing binary builds (check dpkg-buildpackage.1 for how to do that) or if all else fails temporarily switching the package to be format=3.0 (native) instead of (quilt). [1]: Dealing with changes that need both upstream and Debian adjustments is probably where git-debrebase would come in handy as it (IIRC) can seperate those a mixed upstream+Debian commit into seperate ones and then you can easily get rid of the Debian specific stuff during the final rebase. You can also just side-step that problem by committing such things separately to begin with. Re-ordering is easy whenever commits are non-overlapping after all. > > > Dne 28.05.2024 (tor) ob 19:23 +0200 je Daniel Gröber napisal(a): > > > > I'm not super happy with the approach of putting git-subrepo.d inside > > > > /usr/share/git-subrepo tbh. I might be able to let it pass but it seems > > > > lintian found another issue that needs patching anyway so you may as > > > > well > > > > fix this too. > > > > I'm still not seeing a change to remove git-subrepo.d. My comments don't > > just go away by ignoring them :P > > > I am sorry that you felt ignored. I simply thought i have a choice here and > tbh > i still do not see what could be wrong by having sourced only scripts in a > separate subdirectory. I'd be happy to understand your git-subrepo.d concerns > better to be able to fully support this decision. No worries. You do have a choice but you should at least communicate why you don't want to do what I suggest i.e. all review comments should be responded to with something lik ACK or NACK+further-discussion. It's not so much about "something wrong" as it is just a matter of having tidy and predictable structure. > > I noticed another thing, we disable the test suite for what appear to > > be trivial reasons. I really don't like maintaining packages without a > > test suite so this is a no-go. Please look into why it's not working > > and fix it with more upstreamable patches if necessary. FYI: Do feel free to ask for help with this one, I realize it's a big task. I got frustrated with the failures and just sent a review instead ;) I'm sure we can get this done working together tho. Below you're not ACK'ing some of my comments again. With email review you really kind of have to say something about each comment otherwise it's really hard to tell if you just missed one or not and it's easier to discuss any disagreements sooner (when I have forgotten less of the context :x) rather than later when you send the next version. > > > does not automatically integrate git-subrepo into git's own bash- > > > completion. This change moves git-subrepo executables into > > > /usr/share/git-subrepo and adds a symlink of its main executable > > > script into /usr/bin to achieve recognition of the 'git subrepo' > > > sub-command under the git bash-completion (through git's: > > > --list-cmds=...,other,...). > > > > > > Gbp-Pq: Name 0001-Fixed-bash-completion-integration-with-git.patch > > > --- > > > Makefile | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 79898f5..3d84454 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -17,7 +17,7 @@ SHARE = share > > > > > > # Install variables: > > > PREFIX ?= /usr/local > > > -INSTALL_LIB ?= $(DESTDIR)$(shell git --exec-path) > > > +INSTALL_LIB ?= $(DESTDIR)$(PREFIX)/share/$(NAME) > > > > While we're fixing upstream's mess $(DESTDIR) should be interpolated in the > > install target only so overriding the directory structure is easier. > > > > The install target should look something like this, vars listed for > > clarity: > > > > ``` > > PREFIX ?= /usr/local > > INSTALL_LIB ?= $(PREFIX)/share/$(NAME) > > install: > > $(INSTALL) -d -m 0755 $(DESTDIR)$(INSTALL_LIB)/ > > $(INSTALL) -C -m 0755 $(LIB) $(DESTDIR)$(INSTALL_LIB)/ > > ``` > > > > Notice the canonical use of $(INSTALL) instead of the plain command, > > doesn't make a difference in this case but it's good Makefile hygiene. > > > > With that setup we can just export the INSTALL_* vars in debian/rules to > > override them: > > > > export INSTALL_LIB=/usr/share/git-subrepo > > export INSTALL_EXT=$(INSTALL_LIB) > > > > Setting INSTALL_EXT equal to INSTALL_LIB gets rid of the git-subrepo.d as > > I've been requesting. > > > > I'm sending you the full patch "Drop unecessary subdir in usr/share" I used > > to test this seperately, lets see if you can figure out how to apply it to > > your repo ;) > > > > You still have to add a seperate commit to make the $(DESTDIR) adjustment. > > > > > INSTALL_EXT ?= $(INSTALL_LIB)/$(NAME).d > > > INSTALL_MAN1 ?= $(DESTDIR)$(PREFIX)/share/man/man1 > > > > > > @@ -67,6 +67,8 @@ install: > > > install -C -m 0755 $(EXTS) $(INSTALL_EXT)/ > > > install -d -m 0755 $(INSTALL_MAN1)/ > > > install -C -m 0644 $(MAN1)/$(NAME).1 $(INSTALL_MAN1)/ > > > + install -d -m 0755 $(DESTDIR)$(PREFIX)/bin/ > > > + ln -s ../share/$(NAME)/$(NAME) $(DESTDIR)$(PREFIX)/bin/$(NAME) > > > > Creating symlinks like this we'd usually do with dh_link(1). This would be > > OK if you're intending to send this patch upstream? > > > > > > > > # Uninstall support: > > > uninstall: > > > -- > > > 2.39.2 > > > > > > From: Samo Pogačnik <samo_pogac...@t-2.net> > > > > > > Bash scripts under git-suberpo.d are not to be executed standalone > > > therefore should not have executable permissions. > > > > > > Gbp-Pq: Name > > > 0002-Removed-executable-permission-on-sourced-only-files.patch > > > --- > > > Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 3d84454..818cd7d 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -64,7 +64,7 @@ install: > > > install -C -m 0755 $(LIB) $(INSTALL_LIB)/ > > > sed -i 's!^SUBREPO_EXT_DIR=.*!SUBREPO_EXT_DIR=$(INSTALL_EXT)!' > > > $(INSTALL_LIB)/$(NAME) > > > install -d -m 0755 $(INSTALL_EXT)/ > > > - install -C -m 0755 $(EXTS) $(INSTALL_EXT)/ > > > + install -C -m 0644 $(EXTS) $(INSTALL_EXT)/ > > > > This sort of thing would usually be done in debian/rules to keep things > > maintainable. Again if the patch is intended to go upstream it's > > acceptable. > > > > > install -d -m 0755 $(INSTALL_MAN1)/ > > > install -C -m 0644 $(MAN1)/$(NAME).1 $(INSTALL_MAN1)/ > > > install -d -m 0755 $(DESTDIR)$(PREFIX)/bin/ > > > -- > > > 2.39.2 > > > > > > > > > > From: Samo Pogačnik <samo_pogac...@t-2.net> > > > > > > Sourced-only scripts should not start with hashbangs. > > > > > > Gbp-Pq: Name 0003-Removed-hashbangs-from-sourced-only-files.patch > > > --- > > > lib/git-subrepo.d/help-functions.bash | 2 -- > > > pkg/bin/generate-completion.pl | 2 -- > > > pkg/bin/generate-help-functions.pl | 2 -- > > > share/completion.bash | 2 -- > > > 4 files changed, 8 deletions(-) > > > > > > diff --git a/lib/git-subrepo.d/help-functions.bash > > > b/lib/git-subrepo.d/help- > > > functions.bash > > > index 123bb54..6dd5c17 100644 > > > --- a/lib/git-subrepo.d/help-functions.bash > > > +++ b/lib/git-subrepo.d/help-functions.bash > > > @@ -1,5 +1,3 @@ > > > -#!/usr/bin/env bash > > > - > > > # DO NOT EDIT. This file generated by pkg/bin/generate-help-functions.pl. > > > > Should we not fix pkg/bin/generate-help-functions.pl too/instead ? > > > > The header does say not to edit this ;) > > > > > > > > set -e > > > diff --git a/pkg/bin/generate-completion.pl > > > b/pkg/bin/generate-completion.pl > > > index 7ba4165..c4aedb2 100644 > > > --- a/pkg/bin/generate-completion.pl > > > +++ b/pkg/bin/generate-completion.pl > > > @@ -156,8 +156,6 @@ sub generate_bash { > > > } > > > > > > print <<"..."; > > > -#!bash > > > - > > > # DO NOT EDIT. This file generated by pkg/bin/generate-completion.pl. > > > > > > _git_subrepo() { > > > diff --git a/pkg/bin/generate-help-functions.pl b/pkg/bin/generate-help- > > > functions.pl > > > index 40b6af8..dcd3b27 100644 > > > --- a/pkg/bin/generate-help-functions.pl > > > +++ b/pkg/bin/generate-help-functions.pl > > > @@ -29,8 +29,6 @@ sub main { > > > > > > sub write_start { > > > print <<'...'; > > > -#!/usr/bin/env bash > > > - > > > # DO NOT EDIT. This file generated by pkg/bin/generate-help-functions.pl. > > > > > > set -e > > > diff --git a/share/completion.bash b/share/completion.bash > > > index adaa9c6..8a89a72 100644 > > > --- a/share/completion.bash > > > +++ b/share/completion.bash > > > @@ -1,5 +1,3 @@ > > > -#!bash > > > - > > > # DO NOT EDIT. This file generated by pkg/bin/generate-completion.pl. > > > > > > _git_subrepo() { > > > -- > > > 2.39.2 > > > > > > > Thanks, > > --Daniel > > --Daniel
signature.asc
Description: PGP signature
