Hi Samo, You force-pushed again. I know I had notes on your commit hygene, but after uploading we're in maintanance mode now so you really must not do that. The notes were intended as feedback for future commits only.
Luckily I hadn't pushed the three commit at issue on to salsa yet so no
harm done -- and even if we could have handled it, but it's not
proper. Just because I'm still in the middle of your git path to salsa
there's no reason not to treat your repo as if it's the canonical one.
On Thu, Aug 22, 2024 at 10:34:47PM +0200, Samo Pogačnik wrote:
> > 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.
Perhaps I'm just over-pedantic as per ususal. I just found the commit
message uninformative I think.
> And both generating and cleaning should have been done upstream at the
> end, i believe.
Agree.
> Do you see mixing generating/cleaning and moving/restoring as juggling
> upstream and downstream changes?
The "juggling" comment was to say that I understand if you're too lazy to
split it up properly, I would have probably done the same when this isn't a
Debian-only commit but rather one that's going to upstream.
> > 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?
I found the problem. I have
$ git config --global pull.ff only
$ git config --global merge.ff only
Yeah the current testsuite setup is just not going to do. 1) We really have
to override the user config to have any chance at reproducibility or make
git-subrepo more robust in the face of things like this.
2) The testsuite quashes the error output that would have made debugging
this trivial. There should be some kind of VERBOSE= toggle to get it when
debugging. That's something to discuss with upstream.
The current test setup is a mess so this is kind of difficult. The clean
way to do it would be to remove the assumption that there is a pre-existing
.git entirely, but that may make yet more upstream TODOs.
One trick you can try to use is to override the git command in the
test/setup script like so:
git () { ENV=whatever command git -c 'some confg' "$@"; }
read bash -c 'help command' if you're confused and note that shell
functions have precedence over $PATH search. You can use this primitive to
alleviate the leakage of ~/.gitconfig into the tests or just to move the
.git dir in use to a tmp dir (cf. --git-dir or GIT_DIR envvar. See
git.1). You can also override $HOME to get git to look in a different place
for ~/.gitconfig. Much possibility, very wow.
If all you end up needing is setting ENVvars ofc. a simple `export
ENV=whatever` in test/setup would also do. Oh there's also
GIT_CONFIG_GLOBAL so maybe the -c overrides aren't necessary.
-----
Ok the below is all pointless now since I found the bug, but for posperity:
How to repro with Debvm (thanks Helmut <3):
$ debvm-create -o sid.debvm -r sid -z 20G -k ~/.ssh/authorized_keys --
--hook-dir=/usr/share/mmdebstrap/hooks/useradd
--hook-dir=/usr/share/mmdebstrap/hooks/9pmount
--aptopt='Apt::Install-Recommends "true"'
--include=linux-image-generic,build-essential,devscripts,git,quilt,sudo
$ debvm-run -i sid.debvm -s 2222 -- -m 2G -virtfs
local,security_model=none,path=/home,mount_tag=home
# wait for testvm prompt. your system /home/$USER is at /media/home/$USER
if needed
testvm~# su -l user
testvm~$ git clone https://salsa.debian.org/spog/git-subrepo.git
testvm~$ cd git-subrepo/
testvm~/git-subrepo$ origtargz
testvm~/git-subrepo$ debuild -uc -us #< seems to work fine
# I see the second test, branch-rev-list-one-path.t fail when the bugs
reproduce
# Ctrl-C, ok let's try schroot then
testvm~$ sudo apt-get install sbuild-debian-developer-setup
testvm~$ sudo sbuild-debian-developer-setup
testvm~/git-subrepo$ debuild -- clean
testvm~/git-subrepo$ sbuild
# also works .... hmmm
> > 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.
If you're adapting to upstream code-style (as you should) that's fine
then. Looks like a neat hack I'm just not familiar with the edge-case
behavour of it (and am too lazy to think through it rn) is all.
> > 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?
No, that would have only made sense if you *are not* targetting upstream
with your patches. I'm trying to teach you general concepts, it's not
always a comment on what to do in git-subrepo in particular ;)
The do-it-all-in-patches approach is fine for us here, but other situations
may call for different strategy.
On Sun, Aug 25, 2024 at 02:51:20PM +0200, Samo Pogačnik wrote:
> 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.
No need. I have a halfway decent MUA. I can reply to multiple mails at once :O
Thanks,
--Daniel
eagerly awaiting more patches.
signature.asc
Description: PGP signature
