Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IModule::getDependencyFilePath returns relative path for shaders in cwd #5332

Closed
tomas-davidovic opened this issue Oct 17, 2024 · 5 comments · Fixed by #5553
Closed

IModule::getDependencyFilePath returns relative path for shaders in cwd #5332

tomas-davidovic opened this issue Oct 17, 2024 · 5 comments · Fixed by #5553
Assignees
Labels
goal:client support Feature or fix needed for a current slang user. kind:bug something doesn't work like it should

Comments

@tomas-davidovic
Copy link

For hot reload in SGL, we are pulling on all file dependencies of a slang module to find all the directories we need to watch for changes.
The IModule::getDependencyFilePath gives up a bit of a mixed bag of results.

It returns:

  • Absolute paths for most of the files (what we'd expect)
  • Relative paths for shaders that are in the current working directory (didn't expect that)
  • Strings like cbe84a99d1f8c9bdc0f6cc4ffa3b612576d8bd1f, probably for string modules (as opposed to file modules), would expect a nullptr here as there is no FilePath.

Could you please shed some light on the expected behavior? From our side, the ideal behavior would be nullptr for modules with no file, and absolute filepath for all file based modules, but even a more detailed description of the possible outputs would help catch it all.

We do have a workaround, if the path is not absolute, we try to look for the returned string as a file in cwd, and if it is not there, we assume it is a string based module and skip, but we're not sure we have all the possible options covered.

@csyonghe
Copy link
Collaborator

In Slang, every module is associated with a file path, that file path can be a real physical path, or an imaginary one.

Slang cannot tell if a path is real or not. After all, when you load a module from source, you also get to specify the file path for the source that slang will use to identify this module. It makes sense for slang to return back whatever is being passed to the filepath parameter at loadModuleFromSource when you query file dependencies.

It is therefore the application's responsibility if they want to make sure a file path slang returns is an actual file.

@csyonghe
Copy link
Collaborator

In addtion, slang allows applications to pass in a custom IFileSystem interface to handle all file load/store requests. The path can be completely arbitrary/imaginary as long as the user provided file system handler can deal with them. There isn't a canonical way in Slang codebase to tell what accounts a valid path and what does not.

@csyonghe
Copy link
Collaborator

Because of the restricted view of the file system through the IFileSystem interface, it is also not possible for slang to canonical all paths into absolute paths. It is dependent on where the path comes from and slang itself has no knowledge to convert the path names.

@csyonghe
Copy link
Collaborator

I am closing this issue now, since I don't see any good actionable items. I think your workaround makes sense and feels the right way to address the issue.

@tomas-davidovic
Copy link
Author

I am gonna ping this again, since I was digging a bit deeper after I did my workaround.

My problem comes from IncludeSystem::findFile.
It first tries to find the file relative to the current path, and if it can, it just returns it like that.
If it cannot, it will then try to prepend search paths to it, turning the path into absolute path.

In either case, it then goes and converts to absolute path for uniqueIdentity, but when asking for Module::getDependencyFilePath I get the "found path" which is "either relative or absolute, depending on the cwd" and not the uniqueIndentity, which is always absolute.

I don't think the result of foundPath should be change based on cwd. I'd just convert the path to absolute path always, even when found without using the search path (e.g., in STL I'd just call std::filesystem::absolute on the result).
However, I do not know how is IncludeSystem related to the IFileSystem and how much could this change break the alternative implementations.

@csyonghe csyonghe reopened this Nov 7, 2024
@bmillsNV bmillsNV added the kind:bug something doesn't work like it should label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:client support Feature or fix needed for a current slang user. kind:bug something doesn't work like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants