[WTF] Add getting/changing current working directory to FileSystem

• Component: Web Template Framework
• Status: RESOLVED INVALID

[Stephan Szabo]

Add the ability to get and change the current working directory to FileSystem for use inside the jsc shell.

[Stephan Szabo]

Created attachment 361321 [details]
Add get/change current working directory functions

Initial upload to check behavior on gtk/mac on ews.

[Christopher Reid]

Comment on attachment 361321 [details]
Add get/change current working directory functions

View in context: https://bugs.webkit.org/attachment.cgi?id=361321&action=review

> Source/WTF/wtf/glib/FileSystemGlib.cpp:470
> +    return stringFromFileSystemRepresentation(g_get_current_dir());

I think this leaks the returned string. The glib docs for g_get_current_dir states "The returned string should be freed when no longer needed"

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:500
> +        return "";

Should we return Optional<String> to avoid something like getCurrentWorkingDirectory() + "/file.txt" inadvertently attempt to create a file in the root?

[Michael Catanzaro]

Comment on attachment 361321 [details]
Add get/change current working directory functions

View in context: https://bugs.webkit.org/attachment.cgi?id=361321&action=review

> Source/WTF/wtf/FileSystem.h:191
> +WTF_EXPORT_PRIVATE bool changeCurrentWorkingDirectory(const String &);

String&

>> Source/WTF/wtf/glib/FileSystemGlib.cpp:470
>> +    return stringFromFileSystemRepresentation(g_get_current_dir());
> 
> I think this leaks the returned string. The glib docs for g_get_current_dir states "The returned string should be freed when no longer needed"

Yeah, should be:

 GUniquePtr<char> currentDir(g_get_current_dir());
 return stringFromFileSystemRepresentation(currentDir.get());

> Source/WTF/wtf/glib/FileSystemGlib.cpp:473
> +bool changeCurrentWorkingDirectory(const String &filePath)

String&

[Stephan Szabo]

(In reply to Christopher Reid from comment #2)
> Comment on attachment 361321 [details]
> Add get/change current working directory functions
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361321&action=review


> > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:500
> > +        return "";
> 
> Should we return Optional<String> to avoid something like
> getCurrentWorkingDirectory() + "/file.txt" inadvertently attempt to create a
> file in the root?

I'd chosen just returning String because that's what homeDirectoryPath was doing, but making it an Optional is probably better. Will change, thanks.

[Stephan Szabo]

Created attachment 361404 [details]
Add get/change current working directory functions, fix a few issues

Switching get to return Optional<String>, fix memory handling for GLib implementation.

[Darin Adler]

Comment on attachment 361404 [details]
Add get/change current working directory functions, fix a few issues

Why do we want to add these?

It seems dangerous to have any code in WebKit that changes the working directory as a side effect, or to have code that depends on the current working directory. Maybe we need this for non-UI processes?

It’s nice to have a good implementation of these functions if they are needed for cross-platform code that we want to write in the WebKit project, but I don’t yet understand where that code would be.

[Darin Adler]

Comment on attachment 361404 [details]
Add get/change current working directory functions, fix a few issues

Oh, I see "for use in the jsc shell". Seems a shame to have to have this in WTF just because of that, but I guess I can live with it.

[Darin Adler]

Comment on attachment 361404 [details]
Add get/change current working directory functions, fix a few issues

View in context: https://bugs.webkit.org/attachment.cgi?id=361404&action=review

I’d like to see the one first use of this in JSC in the same patch as introducing it. Not a huge fan of adding something unused to WTF.

> Source/WTF/wtf/glib/FileSystemGlib.cpp:470
> +    GUniquePtr<char> currentDir(g_get_current_dir());

When does this give a different value than getcwd?

> Source/WTF/wtf/glib/FileSystemGlib.cpp:476
> +    CString fsRep = fileSystemRepresentation(filePath);

Seems wasteful to have a local variable here.

> Source/WTF/wtf/glib/FileSystemGlib.cpp:477
> +    return !chdir(fsRep.data());

This is the same as the POSIX one. Maybe we can rearrange things so we don’t have two copies. Does this even work when glib is running on Windows?

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:506
> +    CString fsRep = fileSystemRepresentation(filePath);

Seems wasteful to have a local variable here. Just do it all on one line.

[Stephan Szabo]

(In reply to Darin Adler from comment #8)
> Comment on attachment 361404 [details]
> Add get/change current working directory functions, fix a few issues
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361404&action=review
> 
> I’d like to see the one first use of this in JSC in the same patch as
> introducing it. Not a huge fan of adding something unused to WTF.

Okay, will make up a version with the JSC stuff as well.

> > Source/WTF/wtf/glib/FileSystemGlib.cpp:470
> > +    GUniquePtr<char> currentDir(g_get_current_dir());
> 
> When does this give a different value than getcwd?

It appears that it does something different on Windows and elsewhere will do some extra things with the PWD environment variable if this is correct: https://github.com/GNOME/glib/blob/master/glib/gfileutils.c#L2683
Specifically, it seems like it may handle some link cases different by comparing the device and inode of PWD and "." and using the value in PWD rather than what would be given by getcwd if they're the same?
 
> > Source/WTF/wtf/glib/FileSystemGlib.cpp:477
> > +    return !chdir(fsRep.data());
> 
> This is the same as the POSIX one. Maybe we can rearrange things so we don’t
> have two copies. Does this even work when glib is running on Windows?

It probably would, but I think they've marked it as deprecated. While looking up the above it seems like the glib get function is doing windows specific stuff, so it probably should actually use SetCurrentDirectoryW on Windows instead.

[Stephan Szabo]

Created attachment 361644 [details]
Add get/change current working directory functions

[Stephan Szabo]

Created attachment 361645 [details]
Add get/change current working directory functions, with JSC changes

This version includes an initial version with the JSC changes to use FileSystem (and these functions for getting/setting working directory). Those are more in flux, but this indicates the intended direction.

[Darin Adler]

Comment on attachment 361645 [details]
Add get/change current working directory functions, with JSC changes

View in context: https://bugs.webkit.org/attachment.cgi?id=361645&action=review

Still not sure that we need a working directory abstraction. If we want "jsc" to have a working directory concept we can simulate that with a global string inside the tool on all platforms, not just PlayStation. No reason we have to use the "real" working directory, except to initialize the tool on startup.

> Source/JavaScriptCore/jsc.cpp:889
> +    FileSystem::PlatformFileHandle handle = FileSystem::openFile(fileName, FileSystem::FileOpenMode::Read);

Here’s a place where "auto" can help a bit. Nice to not have to write out FileSystem::PlatformFileHandle.

> Source/JavaScriptCore/jsc.cpp:902
> +static bool fillBufferWithContentsOfFile(FileSystem::PlatformFileHandle &handle, Vector& buffer)

This function is pretty dangerous in the case where the vector is not of a type that is a single byte.

> Source/JavaScriptCore/jsc.cpp:912
>      buffer.resize(bufferCapacity + initialSize);

Should be grow instead of resize for better efficiency.

> Source/JavaScriptCore/jsc.cpp:915
> +    int readSize = FileSystem::readFromFile(handle, data, bufferCapacity);

I’d suggest auto for readSize.

> Source/JavaScriptCore/jsc.cpp:2753
> +        static const unsigned workingDirectoryStrLength = strlen("--working-directory=");
> +        if (!strncmp(arg, "--working-directory=", workingDirectoryStrLength)) {
> +            m_workingDirectory = String(arg + workingDirectoryStrLength);
> +            continue;
> +        }

Could add a comment here explaining this option is provided primarily for the benefit of platforms that don’t have a working directory concept at the shell level (PlayStation).

> Source/WTF/wtf/FileSystem.h:190
> +WTF_EXPORT_PRIVATE Optional<String> getCurrentWorkingDirectory();

WebKit coding style means we would not use the word "get" in the name of this function. It should be called currentWorkingDirectory or workingDirectory.

> Source/WTF/wtf/FileSystem.h:191
> +WTF_EXPORT_PRIVATE bool changeCurrentWorkingDirectory(const String&);

Not 100% sure we need the word "current" in this function name.

[Stephan Szabo]

(In reply to Darin Adler from comment #12)
> Comment on attachment 361645 [details]
> Add get/change current working directory functions, with JSC changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361645&action=review
> 
> Still not sure that we need a working directory abstraction. If we want
> "jsc" to have a working directory concept we can simulate that with a global
> string inside the tool on all platforms, not just PlayStation. No reason we
> have to use the "real" working directory, except to initialize the tool on
> startup.

True, but I think we'd need to convert relative paths given to things like the command line or functionLoad to absolute paths as well to make that work, since that's the directory we want to the file to load relative to. Here we were just doing that for PlayStation locally in the FileSystem implementation.

I'm making a parallel version where I try to do this instead.

> [snip other items]

I'm also updating for each of these depending on what happens with the above.

[Saam Barati]

(In reply to Stephan Szabo from comment #0)
> Add the ability to get and change the current working directory to
> FileSystem for use inside the jsc shell.

What's the plan on how to use this?

[Stephan Szabo]

(In reply to Saam Barati from comment #14)
> (In reply to Stephan Szabo from comment #0)
> > Add the ability to get and change the current working directory to
> > FileSystem for use inside the jsc shell.
> 
> What's the plan on how to use this?

Although I'm now also looking at different ways based on the earlier responses, the plan was basically:
* Replace the use of direct system calls for the working directory in currentWorkingDirectory() to use the retrieval function in FileSystem.
* Add an option to jsc to set the working directory for systems without a shell that has the notion using the change function in FileSystem.
* Move from direct fopen/fseek/etc to FileSystem calls for reading files in the jsc shell.
* For PlayStation, because it needs absolute paths, use the given working directory if one exists to prefix relative paths in FileSystem before calling down to libc or posix functions.

The "with JSC changes" attachment isn't fully right, but it should give a general idea of what it would look like.

From the earlier responses, I'm now looking to try a version for the shell that:
* Keeps a working directory in jsc
* Sets it from the system on start
* Add option to set it for systems that don't have one
* Convert paths to absolute paths for loading files.

If that works more reasonably, then this will go away.

[Stephan Szabo]

I've added https://bugs.webkit.org/show_bug.cgi?id=194542 for a keeping the --working-directory argument and switching to absolute paths when it's present.