-
Notifications
You must be signed in to change notification settings - Fork 109
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
Update save_images.tcl #256
Conversation
@chetanyagoyal this does fix the issue. Its like chicken and egg problem.. Let this update get merged so that the docker image (stable tag) will get updated with the latest openroad. |
@msaligane please merge this so that daily CI will take latest save_image.tcl file and run it with the latest openroad version. |
OpenFASOC/openfasoc/generators/temp-sense-gen/flow/scripts/final_report.tcl Lines 60 to 63 in 25a97ea
These lines (which call the script) suggest that this script should not be run on the CI. Is it configured improperly perhaps? |
Why do you think this should not be run on the CI? |
|
I agree that this shouldn't be run on the CI. Don't think there's anything wrong with the commands per se. Maybe we could add a condition in the flows that asks it to bypass this statement? |
From the idea of the saving images and the GUI, it makes sense. But since
openroad is not throwing any error even though the command is executed
(remember that you cannot launch gui using this conda package) and the
check is able to catch the command changes, I think we should keep it as it
is.
…On Mon, 30 Oct, 2023, 9:36 pm Chetanya, ***@***.***> wrote:
@chetanyagoyal <https://github.com/chetanyagoyal> this does fix the
issue. Its like chicken and egg problem.. Let this update get merged so
that the docker image (stable tag) will get updated with the latest
openroad.
https://github.com/idea-fasoc/OpenFASOC/blob/25a97ea8eb57669582ddf5ab944edf067a713805/openfasoc/generators/temp-sense-gen/flow/scripts/final_report.tcl#L60-L63
These lines (which call the script) suggest that this script should not be
run on the CI. Is it configured improperly perhaps?
Why do you think this should not be run on the CI?
gui::show probably means that it opens a GUI window. Also the comment
above says that it should only run if OpenROAD is compiled with GUI.
@chetanyagoyal <https://github.com/chetanyagoyal> this does fix the
issue. Its like chicken and egg problem.. Let this update get merged so
that the docker image (stable tag) will get updated with the latest
openroad.
https://github.com/idea-fasoc/OpenFASOC/blob/25a97ea8eb57669582ddf5ab944edf067a713805/openfasoc/generators/temp-sense-gen/flow/scripts/final_report.tcl#L60-L63
These lines (which call the script) suggest that this script should not be
run on the CI. Is it configured improperly perhaps?
I agree that this shouldn't be run on the CI. Don't think there's anything
wrong with the commands per se. Maybe we could add a condition in the flows
that asks it to bypass this statement?
—
Reply to this email directly, view it on GitHub
<#256 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AYCOBKIPIFNQPZLYA3RZCCLYB7GAPAVCNFSM6AAAAAA6RP3D5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBVGU2DIMRVGM>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Thanks @chetanyagoyal . Tomorrow's check from the bot will help us test the latest openroad version with this update |
Fixes #254.
Updates the save_images.tcl file to accommodate for the changes made in Openroad-flow-scripts