Skip to content

src: fix PyMain arguments forwarding#2288

Merged
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
proppy:fix-python
Sep 22, 2022
Merged

src: fix PyMain arguments forwarding#2288
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
proppy:fix-python

Conversation

@proppy
Copy link
Copy Markdown
Contributor

@proppy proppy commented Sep 21, 2022

This copy all the arguments, instead of just forwarding argv[0], reusing the same code that was previously in 47e8b5f#diff-7c2e2869b9670e340a14a2b2ed1cc45a9f85fb8b219a4c1f06fdd467f49375c1L216-L222

Fixes: #2287

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

PTAL, signed the commit so that DCO check passes.

@maliberty
Copy link
Copy Markdown
Member

-exit handles this case as mentioned in the issue so I see no need for this PR. Do you disagree?

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

I guess I'm confused because OpenLane doesn't seem to be passing -exit to any of those invocation:
https://github.com/The-OpenROAD-Project/OpenLane/search?q=%24%3A%3Aenv%28OPENROAD_BIN%29

Are you saying that they depends on a buggy behavior? (i.e: the fact that before 47e8b5f#diff-7c2e2869b9670e340a14a2b2ed1cc45a9f85fb8b219a4c1f06fdd467f49375c1L216-L222 openroad didn't drop into an interactive shell if -exit was omitted)

@maliberty
Copy link
Copy Markdown
Member

It may have been buggy before but I believe this is the correct behavior. This is how OR works in tcl mode so I would expect it to be the same with -python.

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

It may have been buggy before

So would you recommend instead to do a PR against OpenLane to add -exit flag everywhere they invoke OpenROAD?

@maliberty
Copy link
Copy Markdown
Member

It may have been buggy before

So would you recommend instead to do a PR against OpenLane to add -exit flag everywhere they invoke OpenROAD?

I see for tcl they use run_openroad_script which handles adding -exit. I don't know if that should be adapted to work with python or what you suggest is ok. Best to ask @donn

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

Lets' continue the discussion here:
The-OpenROAD-Project/OpenLane#1387

Comment thread src/Main.cc Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually without this change, the python scripts wouldn't be able to process any args (since only the .py file would be passed to PyMain).

Is that intentional? (i.e: it was never supposed to be a supported usecase) because OpenLane seems to rely on this behavior quite a bit.

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

As commented in The-OpenROAD-Project/OpenLane#1388 (comment), reopening to continue to discuss this, as I can't seem to find a good way to pass -exit to an openroad -python script.py invocation that also expect to process additional flags in python.

@maliberty
Copy link
Copy Markdown
Member

OR doesn't take script arguments in TCL. It does seem like a nice capability though. If you want to rework this PR to take arguments but still respect the -exit convention (ie without it the tool stays open) I am open to it.

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 21, 2022

OR doesn't take script arguments in TCL. It does seem like a nice capability though

It did seem that version <47e8b5f#diff-7c2e2869b9670e340a14a2b2ed1cc45a9f85fb8b219a4c1f06fdd467f49375c1L216-L222 supported it, which apparently ended up having OpenLane depend on it.

Happy to re-work the PR, but it's unfortunate that OpenLane is broken with HEAD. It does seems that some CI check failed: https://jenkins.openroad.tools/job/OpenLane-Public/1300/ but I'm curious why it isn't surfaced on #1849 as it seems useful (even just as a FYI) to understand that a change break a downstream package.

@maliberty
Copy link
Copy Markdown
Member

There is a CI problem but it isn't related to the python PR but to sta updates

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 22, 2022

PTAL, added support for -exit and flag forwarding to -python to restore the previous behavior that OpenLane depends on (and allow us to proceed with The-OpenROAD-Project/OpenLane#1388).

tests

🚫 script, 🚫 arguments, 🚫 -exit

OpenROAD 🧁 ./build/src/openroad -python 
OpenROAD v2.0-4989-gb3821e409 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
Python 3.10.7 (main, Sep  8 2022, 14:34:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 

🟢 script, 🚫 arguments, 🚫 -exit

OpenROAD 🍧 cat foo.py 
import sys
print('hello from openroad', sys.argv)
OpenROAD 🧁 ./build/src/openroad -python foo.py
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']
Python 3.10.7 (main, Sep  8 2022, 14:34:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 

🟢 script, 🟢 arguments, 🚫 -exit

OpenROAD 🍧 ./build/src/openroad -python foo.py a b -c
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py', 'a', 'b', '-c']
Python 3.10.7 (main, Sep  8 2022, 14:34:29) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 

🟢 script, 🚫 arguments, 🟢 -exit

OpenROAD 🍙 ./build/src/openroad -python foo.py -exit
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']

🟢 script, 🟢 arguments, 🟢 -exit

OpenROAD 🍙 ./build/src/openroad -python foo.py -exit
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']

Signed-off-by: Johan Euphrosine <proppy@google.com>
@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 22, 2022

rebased, with DCO signature.

@maliberty
Copy link
Copy Markdown
Member

Does the order of args matter ? For example:
./build/src/openroad -python foo.py -exit
./build/src/openroad -exit -python foo.py
./build/src/openroad -python -exit foo.py

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 22, 2022

OpenROAD 🍫 ./build/src/openroad -python foo.py -exit
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']
OpenROAD 🍧 ./build/src/openroad -python -exit foo.py
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']
OpenROAD 🍙 ./build/src/openroad -exit -python foo.py
OpenROAD v2.0-4990-gdf3add309 
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.
[WARNING ORD-0039] .openroad ignored with -python
hello from openroad ['foo.py']

@maliberty maliberty merged commit 5e557ee into The-OpenROAD-Project:master Sep 22, 2022
@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 27, 2022

@tspyrou suggested that we add unittest to validate that openroad -python assumptions hold over time.

Most of the tests in https://github.com/The-OpenROAD-Project/OpenROAD/tree/master/test seems to be "design-oriented"; @maliberty is there already some test infrastructure to put up some functional test for the CLI? or should we create one?

@maliberty
Copy link
Copy Markdown
Member

Define what a 'functional test' means to you. We have tests in every tool, the top level tests are system level tests.

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 27, 2022

Define what a 'functional test' means to you

I test that would encode the behavior in #2288 (comment)

@maliberty
Copy link
Copy Markdown
Member

I don't think we need a whole infrastructure to test command line options. You can make a test like src/drt/test/gc_test.tcl that shells out

@proppy
Copy link
Copy Markdown
Contributor Author

proppy commented Sep 27, 2022

Thanks for the reference! By infrastructure I mostly meant some test structure I can copy ;)

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.

OpenROAD hangs forever when executing python scripts

2 participants