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

-y flag for libyosys Python scripts #4553

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Conversation

donn
Copy link
Contributor

@donn donn commented Aug 19, 2024

What are the reasons/motivation for this change?

This adds a Python equivalent to the -c option, where scripts importing libyosys can be used.

Most of the requisite work for this was already done to enable Python passes a couple years back, so this is a relatively small changeset, and would add another option for supporting libyosys-based scripts (that does not involve PYTHONPATH shenanigans.)

Explain how this is achieved.

Adding a new opt, -y (-p and -P were already taken,) and running PyRun_SimpleFile on scriptfile. Similar to -c for Tcl, any unprocessed arguments are passed to the script in the form of sys.argv.

If applicable, please suggest to reviewers how they can test the change.

./yosys -y <(echo "
import libyosys as ys

d = ys.Design()
ys.run_pass('help', d)
print(sys.path)
print(sys.argv)") -- test argv

@donn
Copy link
Contributor Author

donn commented Aug 25, 2024

Any feedback on this? (The Verific test is failing because of security reasons- I'm a first-time contributor)

@KrystalDelusion
Copy link
Member

Any feedback on this? (The Verific test is failing because of security reasons- I'm a first-time contributor)

That should be fixed by #4552, you may need to merge/rebase on latest main to get the fix though

@donn donn force-pushed the python_scriptfile branch from 1cb7467 to b047cb7 Compare August 27, 2024 05:46
@donn donn force-pushed the python_scriptfile branch 2 times, most recently from f971361 to 83617a6 Compare August 27, 2024 05:50
@donn
Copy link
Contributor Author

donn commented Aug 27, 2024

Rebased on the wrong upstream for a second there but should be fine now :)

@mmicko mmicko added the discuss to be discussed at next dev jour fixe (see #devel-discuss at https://yosyshq.slack.com/) label Sep 4, 2024
@povik povik self-assigned this Sep 9, 2024
@donn donn force-pushed the python_scriptfile branch from 0566f89 to 6c391d2 Compare September 11, 2024 18:45
This adds a Python equivalent to the `-c` option, where scripts importing `libyosys` can be imported and used.

Most of the work for this was already done to enable Python passes a couple years back, so this is a relatively small changeset.
This matches the behavior of running a Python interpreter, where the
first element of sys.path is the dirname of the script being run.

This allows importing of files and modules in the same directory without
messing with PYTHONPATH or similar.
kernel/driver.cc Outdated Show resolved Hide resolved
kernel/driver.cc Show resolved Hide resolved
* Less brittle method of adding script dirname to sys.path
* Check if scriptfp successfully opens before using it
* Move `log_error` to after `PyErr_Print()` is called
@donn donn requested a review from jix September 30, 2024 15:17
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested here, looks good, though I haven't vetted the usage of the CPython API and the error handling paths around it

@povik povik added merge-before-release Merge: PR should be included in the next release and removed discuss to be discussed at next dev jour fixe (see #devel-discuss at https://yosyshq.slack.com/) labels Oct 3, 2024
@widlarizer widlarizer merged commit 1f517d6 into YosysHQ:main Oct 7, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-before-release Merge: PR should be included in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants