On Tue, Dec 1, 2020 at 8:41 PM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Wed, Dec 2, 2020 at 3:00 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> >
> > On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> > > >
> > > > get_absolute_path() makes an attempt to allow for this.
> > > > But that doesn't work as soon as os.chdir() gets called.
> > >
> > > Can we explain why this doesn't work? It's because the test_data/
> > > files are accessed with relative paths, so chdir breaks access to
> > > them, right?
> >
> > Correct.
> > Because it actually returns a relative path.
> >
> > (I forgot that I called out that get_absolute_path() gives relative
> > paths in the last patch, and not this one. I can update the commit
> > desc if we want a v2 of this)
> >
> > >
> > > >
> > > > So make it so that os.chdir() does nothing to avoid this.
> > > >
> > > > Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> > > > the introduction of a new base class instead.
> > > >
> > > > Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> > > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > > > ---
> > >
> > > I don't like this: disabling a real system call seems like overkill to
> > > work around a path issue like this.
> > >
> > > Wouldn't it be better to either:
> > > (a) stop kunit_tool from needing to chdir entirely; or
> >
> > a) is doable, but would require plumbing cwd=get_kernel_root_path() to
> > all the subprocess calls to make, etc.
> > I'm not sure fixing a internal test-only issue necessarily justifies
> > taking that path instead of the easier "just add a chdir" we opted for
> > in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside
> > kernel tree").
> >
> > > (b) have get_absolute_path() / test_data_path() produce an absolute path.
> > >
> > > The latter really seems like the most sensible approach: have the
> > > script read its own path and read files relative to that.
> >
> > b) is not that simple, sadly.
> >
> > Say I invoke
> > $ python3 kunit_tool_test.py
> > then __file__ = kunit_tool_test.py.
> >
> > So __file__ is a relative path, but the code assumed it was an
> > absolute one and any change of directory breaks things.
> > Working around that would require something like caching the result of
> > os.path.abspath(__file__) somewhere.
>
> So, to clarify, __file__ is a relative path based on the cwd when the
> script is initially run, right?
Yes, on my box at least.
https://docs.python.org/3/reference/import.html#__file__ doesn't not
seem to stipulate an absolute path, either.
>
> In any case, caching the result of os.path.abspath(__file__) seems
> like the most sensible solution to me. There's global state anyway
> (the current directory), we might as well have it in an explicit
> variable, IMHO.
Ok, sent out a v2 and squash this change with the test_data_path() patch.
Not really a fan of the adding the global state, but I see your point
about there maybe being need for more chdir calls and I don't see a
better way of keeping track of the absolute path.
> >
> > I can see the point about not mocking out something like os.chdir().
> > But on the other hand, do we have any other legitimate reason to call
> > it besides that one place in kunit.py?
> > If we do have something that relies on doing an os.chdir(), it should
> > ideally notice that it didn't work and manifest in a unit test failure
> > somehow.
>
> Certainly there doesn't seem to be any other need to chdir() in
> kunit_tool at the moment, but I could see us doing so when adding
> other features. More to the point, if both kunit.py and
> kunit_tool_test.py rely on or manipulate the current directory as part
> of their state, that seems like it's asking for some trouble down the
> line.
>
> If we use an absolute path for the test data, that's something that
> seems unlikely to ever need further changes or cause issues.
> >
> > Alternatively, we could make get_kernel_root_path() return ""/None to
> > avoid the chdir call.
> > kunit.py: if get_kernel_root_path():
> > kunit.py: os.chdir(get_kernel_root_path())
> >
> > There'd be no adverse affects atm for stubbing that out, and I don't forsee any.
> > But if we want to be even safer, then perhaps we have
> >
> > def chdir_to_kernel_root():
> > ...
> >
> > and mock out just that func in the unit test?
>
> I'd be okay with this, even if I'd prefer us to use an absolute path
> for the test_data as well. Having something like this might even give
> us the opportunity to verify that we're actually trying to change to
> the kernel directory in cases where we need to, but that seems like
> it's out-of-scope for a simple fix.
>
> -- David
