| Message ID | 20220204105418.30167-1-Cedric_Hombourger@mentor.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3] dpkg-base: resolve DL_DIR in do_adjust_git | expand |
On 04.02.22 11:54, Cedric Hombourger wrote: > From: Cedric Hombourger <cedric.hombourger@siemens.com> > > git_link is resolved using os.path.realpath() but git_dl is not. > If DL_DIR points to a symbolic link, the comparison will always > fail and do_adjust_git() will attempt to re-create the symbolic > link. Resolve DL_DIR for a comparison between resolved paths. > In the event where paths do differ, the symbolic link needs to > be deleted first. > > Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com> > --- > meta/classes/dpkg-base.bbclass | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass > index 2add0b2..258e040 100644 > --- a/meta/classes/dpkg-base.bbclass > +++ b/meta/classes/dpkg-base.bbclass > @@ -20,9 +20,13 @@ python do_adjust_git() { > rootdir = d.getVar('WORKDIR', True) > > git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), '.git-downloads') > - git_dl = os.path.join(d.getVar("DL_DIR"), "git") > + dl_dir = os.path.realpath(d.getVar("DL_DIR")) > + git_dl = os.path.join(dl_dir, "git") > > - if not os.path.exists(git_link) or os.path.realpath(git_link) != git_dl: > + if os.path.exists(git_link) and os.path.realpath(git_link) != git_dl: > + os.unlink(git_link) > + > + if not os.path.exists(git_link): > os.symlink(git_dl, git_link) > > for src_uri in (d.getVar("SRC_URI", True) or "").split(): > @@ -34,7 +38,7 @@ python do_adjust_git() { > > if os.path.islink(ud.localpath): > realpath = os.path.realpath(ud.localpath) > - filter_out = os.path.join(d.getVar("DL_DIR"), "git") + "/" > + filter_out = git_dl + "/" > if realpath.startswith(filter_out): > # make the link relative > link = realpath.replace(filter_out, '', 1) Looks good and was surely an untested case so far. It's actually two fixes in one (os.symlink != ln -sf...) Does this explain your "race condition" from before? Jan
Working with sstate i found a very similiar thing but in a very different place. https://github.com/henning-schild-work/isar/commit/d7eff82427fbe8e4f5ef50268804257fc9c143a0 Makes me wonder who is responsible for creating the link, or if every stakeholder is just hammering around. In fact i still vote for dropping adjust git and using "no shared" cloning. Henning Am Fri, 4 Feb 2022 11:54:18 +0100 schrieb Cedric Hombourger <Cedric_Hombourger@mentor.com>: > From: Cedric Hombourger <cedric.hombourger@siemens.com> > > git_link is resolved using os.path.realpath() but git_dl is not. > If DL_DIR points to a symbolic link, the comparison will always > fail and do_adjust_git() will attempt to re-create the symbolic > link. Resolve DL_DIR for a comparison between resolved paths. > In the event where paths do differ, the symbolic link needs to > be deleted first. > > Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com> > --- > meta/classes/dpkg-base.bbclass | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/meta/classes/dpkg-base.bbclass > b/meta/classes/dpkg-base.bbclass index 2add0b2..258e040 100644 > --- a/meta/classes/dpkg-base.bbclass > +++ b/meta/classes/dpkg-base.bbclass > @@ -20,9 +20,13 @@ python do_adjust_git() { > rootdir = d.getVar('WORKDIR', True) > > git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), > '.git-downloads') > - git_dl = os.path.join(d.getVar("DL_DIR"), "git") > + dl_dir = os.path.realpath(d.getVar("DL_DIR")) > + git_dl = os.path.join(dl_dir, "git") > > - if not os.path.exists(git_link) or os.path.realpath(git_link) != > git_dl: > + if os.path.exists(git_link) and os.path.realpath(git_link) != > git_dl: > + os.unlink(git_link) > + > + if not os.path.exists(git_link): > os.symlink(git_dl, git_link) > > for src_uri in (d.getVar("SRC_URI", True) or "").split(): > @@ -34,7 +38,7 @@ python do_adjust_git() { > > if os.path.islink(ud.localpath): > realpath = os.path.realpath(ud.localpath) > - filter_out = os.path.join(d.getVar("DL_DIR"), "git") > + "/" > + filter_out = git_dl + "/" > if realpath.startswith(filter_out): > # make the link relative > link = realpath.replace(filter_out, '', 1)
On 04.02.22 13:30, Henning Schild wrote: > Working with sstate i found a very similiar thing but in a very > different place. > > https://github.com/henning-schild-work/isar/commit/d7eff82427fbe8e4f5ef50268804257fc9c143a0 That is a re-execution issue of a task, not a race. > > Makes me wonder who is responsible for creating the link, or if every > stakeholder is just hammering around. The first one coming along - one reason why it's under a lock. Jan > > In fact i still vote for dropping adjust git and using "no shared" > cloning. > > Henning > > Am Fri, 4 Feb 2022 11:54:18 +0100 > schrieb Cedric Hombourger <Cedric_Hombourger@mentor.com>: > >> From: Cedric Hombourger <cedric.hombourger@siemens.com> >> >> git_link is resolved using os.path.realpath() but git_dl is not. >> If DL_DIR points to a symbolic link, the comparison will always >> fail and do_adjust_git() will attempt to re-create the symbolic >> link. Resolve DL_DIR for a comparison between resolved paths. >> In the event where paths do differ, the symbolic link needs to >> be deleted first. >> >> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com> >> --- >> meta/classes/dpkg-base.bbclass | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/meta/classes/dpkg-base.bbclass >> b/meta/classes/dpkg-base.bbclass index 2add0b2..258e040 100644 >> --- a/meta/classes/dpkg-base.bbclass >> +++ b/meta/classes/dpkg-base.bbclass >> @@ -20,9 +20,13 @@ python do_adjust_git() { >> rootdir = d.getVar('WORKDIR', True) >> >> git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), >> '.git-downloads') >> - git_dl = os.path.join(d.getVar("DL_DIR"), "git") >> + dl_dir = os.path.realpath(d.getVar("DL_DIR")) >> + git_dl = os.path.join(dl_dir, "git") >> >> - if not os.path.exists(git_link) or os.path.realpath(git_link) != >> git_dl: >> + if os.path.exists(git_link) and os.path.realpath(git_link) != >> git_dl: >> + os.unlink(git_link) >> + >> + if not os.path.exists(git_link): >> os.symlink(git_dl, git_link) >> >> for src_uri in (d.getVar("SRC_URI", True) or "").split(): >> @@ -34,7 +38,7 @@ python do_adjust_git() { >> >> if os.path.islink(ud.localpath): >> realpath = os.path.realpath(ud.localpath) >> - filter_out = os.path.join(d.getVar("DL_DIR"), "git") >> + "/" >> + filter_out = git_dl + "/" >> if realpath.startswith(filter_out): >> # make the link relative >> link = realpath.replace(filter_out, '', 1) >
On Fri, 2022-02-04 at 12:04 +0100, Jan Kiszka wrote: > On 04.02.22 11:54, Cedric Hombourger wrote: > > From: Cedric Hombourger <cedric.hombourger@siemens.com> > > > > git_link is resolved using os.path.realpath() but git_dl is not. > > If DL_DIR points to a symbolic link, the comparison will always > > fail and do_adjust_git() will attempt to re-create the symbolic > > link. Resolve DL_DIR for a comparison between resolved paths. > > In the event where paths do differ, the symbolic link needs to > > be deleted first. > > > > Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com> > > --- > > meta/classes/dpkg-base.bbclass | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg- > > base.bbclass > > index 2add0b2..258e040 100644 > > --- a/meta/classes/dpkg-base.bbclass > > +++ b/meta/classes/dpkg-base.bbclass > > @@ -20,9 +20,13 @@ python do_adjust_git() { > > rootdir = d.getVar('WORKDIR', True) > > > > git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), '.git- > > downloads') > > - git_dl = os.path.join(d.getVar("DL_DIR"), "git") > > + dl_dir = os.path.realpath(d.getVar("DL_DIR")) > > + git_dl = os.path.join(dl_dir, "git") > > > > - if not os.path.exists(git_link) or os.path.realpath(git_link) > > != git_dl: > > + if os.path.exists(git_link) and os.path.realpath(git_link) != > > git_dl: > > + os.unlink(git_link) > > + > > + if not os.path.exists(git_link): > > os.symlink(git_dl, git_link) > > > > for src_uri in (d.getVar("SRC_URI", True) or "").split(): > > @@ -34,7 +38,7 @@ python do_adjust_git() { > > > > if os.path.islink(ud.localpath): > > realpath = os.path.realpath(ud.localpath) > > - filter_out = os.path.join(d.getVar("DL_DIR"), > > "git") + "/" > > + filter_out = git_dl + "/" > > if realpath.startswith(filter_out): > > # make the link relative > > link = realpath.replace(filter_out, '', 1) > > Looks good and was surely an untested case so far. It's actually two > fixes in one (os.symlink != ln -sf...) > > Does this explain your "race condition" from before? Yes it certainly did. I had misinterpreted the symptoms / root-cause (thanks again for pointing me to the lock around do_adjust_git) > > Jan >
Changes since v3: The path placed in .git/objects/info/alternates shall be relative, we should resolve git symbolic links only when comparing them v3 changes: fix subject line v2 changes: found actual root cause. instead of catching FileExistsError, we should instead resolve symbolic links and compare them. In the event when they do not match, the previous symbolic link should be re-created. Cedric Hombourger (1): dpkg-base: resolve DL_DIR in do_adjust_git meta/classes/dpkg-base.bbclass | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Am Fri, 4 Feb 2022 13:30:36 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Working with sstate i found a very similiar thing but in a very > different place. > > https://github.com/henning-schild-work/isar/commit/d7eff82427fbe8e4f5ef50268804257fc9c143a0 > > Makes me wonder who is responsible for creating the link, or if every > stakeholder is just hammering around. > > In fact i still vote for dropping adjust git and using "no shared" > cloning. Yesterday i wanted to backport a patch into a component that already gets patched with isar and git as patchtool. And since that patch did not apply i needed to resolve merge conflicts. Leaving git repos in unusable state on the host is just a massive pain every once in a while. I do not want a devshell in a container, which takes me miles away from where i want to be for a simple "git cherry-pick" because an "git am" did not work. I again ask to switch to "not shared" cloning, or to re-adjust when coming back out of the chroots. Anyone on my side? Last time i proposed patches i was overvoted, but maybe the use-case or pain was not clear. Henning > Henning > > Am Fri, 4 Feb 2022 11:54:18 +0100 > schrieb Cedric Hombourger <Cedric_Hombourger@mentor.com>: > > > From: Cedric Hombourger <cedric.hombourger@siemens.com> > > > > git_link is resolved using os.path.realpath() but git_dl is not. > > If DL_DIR points to a symbolic link, the comparison will always > > fail and do_adjust_git() will attempt to re-create the symbolic > > link. Resolve DL_DIR for a comparison between resolved paths. > > In the event where paths do differ, the symbolic link needs to > > be deleted first. > > > > Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com> > > --- > > meta/classes/dpkg-base.bbclass | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/meta/classes/dpkg-base.bbclass > > b/meta/classes/dpkg-base.bbclass index 2add0b2..258e040 100644 > > --- a/meta/classes/dpkg-base.bbclass > > +++ b/meta/classes/dpkg-base.bbclass > > @@ -20,9 +20,13 @@ python do_adjust_git() { > > rootdir = d.getVar('WORKDIR', True) > > > > git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), > > '.git-downloads') > > - git_dl = os.path.join(d.getVar("DL_DIR"), "git") > > + dl_dir = os.path.realpath(d.getVar("DL_DIR")) > > + git_dl = os.path.join(dl_dir, "git") > > > > - if not os.path.exists(git_link) or os.path.realpath(git_link) > > != git_dl: > > + if os.path.exists(git_link) and os.path.realpath(git_link) != > > git_dl: > > + os.unlink(git_link) > > + > > + if not os.path.exists(git_link): > > os.symlink(git_dl, git_link) > > > > for src_uri in (d.getVar("SRC_URI", True) or "").split(): > > @@ -34,7 +38,7 @@ python do_adjust_git() { > > > > if os.path.islink(ud.localpath): > > realpath = os.path.realpath(ud.localpath) > > - filter_out = os.path.join(d.getVar("DL_DIR"), > > "git") > > + "/" > > + filter_out = git_dl + "/" > > if realpath.startswith(filter_out): > > # make the link relative > > link = realpath.replace(filter_out, '', 1) >
diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass index 2add0b2..258e040 100644 --- a/meta/classes/dpkg-base.bbclass +++ b/meta/classes/dpkg-base.bbclass @@ -20,9 +20,13 @@ python do_adjust_git() { rootdir = d.getVar('WORKDIR', True) git_link = os.path.join(d.getVar('GIT_DL_LINK_DIR'), '.git-downloads') - git_dl = os.path.join(d.getVar("DL_DIR"), "git") + dl_dir = os.path.realpath(d.getVar("DL_DIR")) + git_dl = os.path.join(dl_dir, "git") - if not os.path.exists(git_link) or os.path.realpath(git_link) != git_dl: + if os.path.exists(git_link) and os.path.realpath(git_link) != git_dl: + os.unlink(git_link) + + if not os.path.exists(git_link): os.symlink(git_dl, git_link) for src_uri in (d.getVar("SRC_URI", True) or "").split(): @@ -34,7 +38,7 @@ python do_adjust_git() { if os.path.islink(ud.localpath): realpath = os.path.realpath(ud.localpath) - filter_out = os.path.join(d.getVar("DL_DIR"), "git") + "/" + filter_out = git_dl + "/" if realpath.startswith(filter_out): # make the link relative link = realpath.replace(filter_out, '', 1)