[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via llvm-commits]

haowei added inline comments.


================
Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',
----------------
haowei wrote:
> haowei wrote:
> > bnbarham wrote:
> > > bnbarham wrote:
> > > > I'd prefer a test without `overlay-relative` set to make it clear they don't depend on each other.
> > > There's also unit tests in `llvm/unittests/Support/VirtualFileSystemTest.cpp` that you could add to.
> > I need some time to patch this unit test for testing the new option.
> > Existing functions in this unit test file does not support defining the YAMLFilePath.
> > 
> > I will update this patch when I finish.
> I updated the yaml file and add unit tests with overlay-relative set to true and false.
I updated `llvm/unittests/Support/VirtualFileSystemTest.cpp` to include tests on `root-relative` option.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903
+        assert(!FullPath.empty() && "YAML file directory must exist");
+        sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+        Name = canonicalize(Name);
----------------
bnbarham wrote:
> haowei wrote:
> > bnbarham wrote:
> > > IMO both this and CWD should be using the base FS instead. VFS didn't have a CWD previously, but now that it does it doesn't really make sense to use the process wide CWD. Especially since `-working-directory` doesn't change it.
> > Could you clarify a bit more about the "base FS" please? I am still quite new to the LLVM VFS system.  Which API should I use to get the appropriate working directory instead of the the process wide CWD?
> > 
> > I avoided changing the default behavior (relative to the process's current working directory) as I am a bit concerned breaking other people's use cases. 
> > 
> > I looked up a bit, the value "--working-directory" will be writen into "VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use this value instead of the process current working directory? Though it would still be a behavior change for users rely on process current working directories. 
> > Could you clarify a bit more about the "base FS" please?
> 
> By "base FS" I just meant the filesystem that's passed down when creating the RedirectingFileSystem, ie. `ExternalFS` in `getVFSFromYAML`.
> 
> > Which API should I use to get the appropriate working directory instead of the the process wide CWD?
> 
> `ExternalFS->getCurrentWorkingDirectory()` and `ExternalFS->makeAbsolute` is what I'd expect to be used here.
> 
> > Do you think it is a better idea to use this value instead of the process current working directory? Though it would still be a behavior change for users rely on process current working directories.
> 
> Yes, I think it's surprising that it currently *isn't* using it. It's just a hold over from when the VFS didn't have the concept of CWD. I'd be happy with accepting this patch as is, with a separate patch after that changes to using the VFS CWD.
> Yes, I think it's surprising that it currently *isn't* using it. It's just a hold over from when the VFS didn't have the concept of CWD. I'd be happy with accepting this patch as is, with a separate patch after that changes to using the VFS CWD.

I see, I can upload a follow up change after this one is landed. to use VFS CWD instead of process CWD here. It is also easier to revert if anything goes wrong.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137473/new/

https://reviews.llvm.org/D137473