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

Add opt-in support for resolving relative paths in Yosys scripts based on script location #4571

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RCoeurjoly
Copy link
Contributor

@RCoeurjoly RCoeurjoly commented Aug 29, 2024

What are the reasons/motivation for this change?

This PR addresses the feature described in #4556 by introducing an opt-in mechanism to handle relative file paths in Yosys scripts that are not executed from the current directory.

The opt-in can be controlled from both within and outside the script with a scratchpad variable.

Explain how this is achieved.
When a Yosys script (.ys) located outside the current directory is executed, its directory path is stored in the scratchpad under the key script.dir. This is done unconditionally.

If the scratchpad variable script.relative_path_resolution_enabled is set to true, and the filename to be read/write is relative, the path stored in script.dir is prepended (in a portable manner) to the filename to resolve it correctly.

If you want to disable this behavior and revert to the default handling of relative paths, set the scratchpad variable to 0:
scratchpad -set script.relative_path_resolution_enabled 0

This feature is not available in the WASI build since it lacks support for std::filesystem.
See WASI filesystem status for more details.

If applicable, please suggest to reviewers how they can test the change.
To reproduce the current behaviour, we can run (from yosys root dir) ./yosys tests/opt/opt_expr_cmp.ys and observe the following error:

ERROR: Can't open input file `opt_expr_cmp.v' for reading: No such file or directory

We can enable the feature via command line:
./yosys -p "scratchpad -set script.relative_path_resolution_enabled 1; script tests/opt/opt_expr_cmp.ys"
Or within the script:

cp tests/opt/opt_expr_cmp.ys tests/opt/opt_expr_cmp2.ys
sed -i '1s/^/scratchpad -set script.relative_path_resolution_enabled 1\n/' tests/opt/opt_expr_cmp2.ys
./yosys tests/opt/opt_expr_cmp2.ys

Both ways yosys finds the relevant files correctly.

You can find more .ys scripts to test with the following command (executed from yosys root dir):
find tests -type f -name "*.ys" -exec grep -H "read_" {} + | grep -v -e 'EOT' -e 'EOF' | cut -d':' -f1 | uniq
Note that some of those scripts will fail, for example tests/blif/bug2729.y, due to the limitation regarding shell commands documented below.

Note 1: While mixing absolute and relative paths works correctly, I haven’t found a straightforward way to automate this test in the test suite.

Known limitations

Script calling another script

In such cases, the script.dir variable is overwritten with the directory of the most recently executed script, which can lead to incorrect path resolution. However, this is not a common use case and is unlikely to affect most users.

Shell commands inside script

The relative path resolution mechanism introduced by this feature does not apply to filenames used in shell commands within Yosys scripts. For example, commands like ! rm filename or ! cp filename will not have their paths automatically prefixed or resolved using the script.dir variable.

@Ravenslofty adding you as a reviewer since you seem to be a proponent of the scratchpad: #3880 (comment)

@RCoeurjoly
Copy link
Contributor Author

I think it would be best to make this opt in, otherwise some tests fail such as tests/opt_share/temp/uut_00000.ys.

Also looking into how to fix the WASI build:

error "The library is not supported since libc++ has been configured without support for a filesystem."

@povik
Copy link
Member

povik commented Aug 29, 2024

I am worried this will break existing users, we could make the lookup relative to the script directory be conditioned on some prefix, e.g. ^/

@RCoeurjoly RCoeurjoly marked this pull request as draft August 29, 2024 10:38
@Ravenslofty
Copy link
Collaborator

I suspect what the prefix should be will be very bikesheddable, but a prefix sounds like a good idea.

@Ravenslofty
Copy link
Collaborator

Alternatively, and this is probably also bikesheddable, some sort of command or scratchpad variable or such which asks the Yosys command parser to interpret paths relative to some specified path. That would mean third party scripts could have some equivalent functionality to +/?

@whitequark
Copy link
Member

Also looking into how to fix the WASI build:

error "The library is not supported since libc++ has been configured without support for a filesystem."

At the time there is no std::filesystem on WASI because of thread related issues and most likely there won't be such support for quite a while.

@RCoeurjoly
Copy link
Contributor Author

Alternatively, and this is probably also bikesheddable, some sort of command or scratchpad variable or such which asks the Yosys command parser to interpret paths relative to some specified path. That would mean third party scripts could have some equivalent functionality to +/?

What do you think about adding a -relative_path flag to script? I am bit hesitant about adding some prefix like ^/ to file paths because that would force the user to put potentially a lot of them, instead of just setting one flag.

On the other hand, ^/ would allow per file control of the feature.

Thoughts? @povik @Ravenslofty

@Ravenslofty
Copy link
Collaborator

Speaking from personal experience, when I want to run a script I use yosys -s script.ys, whereas your proposal would require something more like yosys -p "script -relative_path script.ys", and sure, this is relatively minor, but I think it would be painful. I'm not suggesting we add yet another Yosys command line option, to be clear, but I think it's important to take into account when considering implementation ideas.

@povik
Copy link
Member

povik commented Aug 29, 2024

I am bit hesitant about adding some prefix like ^/ to file paths because that would force the user to put potentially a lot of them, instead of just setting one flag.

An alternative is a command like set_cwd -script you would call from the script to opt in having all paths understood relative to the script location.

@povik
Copy link
Member

povik commented Aug 29, 2024

Ah that's what @Ravenslofty suggested. I think we can all agree we want the opt-in controlled from within the script

@RCoeurjoly RCoeurjoly changed the title Prepend yosys script path to relative filenames in script Add opt-in support for resolving relative paths in Yosys scripts based on script location Aug 30, 2024
@RCoeurjoly RCoeurjoly marked this pull request as ready for review August 30, 2024 18:06
@RCoeurjoly RCoeurjoly marked this pull request as draft September 2, 2024 13:28
@RCoeurjoly
Copy link
Contributor Author

In Dev JF we came to the conclusion that we want to support scripts calling other scripts.
For that we would need a stack of script.dir scratchpad variables. Unsure if one script.relative_path_resolution_enabled variable is enough, or one per script, to have more granular control.

@widlarizer widlarizer mentioned this pull request Sep 11, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants