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

OpenROAD: Fix package #366

Merged
merged 5 commits into from
Feb 21, 2024
Merged

OpenROAD: Fix package #366

merged 5 commits into from
Feb 21, 2024

Conversation

ubfx
Copy link
Contributor

@ubfx ubfx commented Jan 23, 2024

Currently, the OpenROAD build fails, because this patch can't be applied.

I actually had to do some more changes in my fork to get the build working again, but I'm not sure how much of that is because of my runner. So maybe this will do.

Fix #368

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

Thanks for the patch! Let me check w/ the CI if it fixes #368

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

still seeing:

2024-02-21T10:08:47.1051513Z �[32;1m10:08:47�[0m | Error was: 
2024-02-21T10:08:47.1053469Z �[32;1m10:08:47�[0m | Command '['/root/conda-eda/conda-eda/workdir/conda-env/bin/patch', '--no-backup-if-mismatch', '--batch', '-Np1', '-i', '/tmp/tmpg6s06yi_/disable-cluster-flops-cmd.patch.native', '--binary', '--dry-run']' returned non-zero exit status 1.

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

Sorry, it seems like the files changed again since I last looked at it. I'm updating the patch now

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

@ubfx thanks for the quick turnaround, running the CI.

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

Looks like we miss an stdlib include?

2024-02-21T11:23:40.7739118Z �[32;1m11:23:40�[0m | /root/conda-eda/conda-eda/workdir/conda-env/conda-bld/openroad_1708513964138/work/src/dpl/src/Opendp.cpp:664:14: error: 'accumulate' is not a member of 'std'
2024-02-21T11:23:40.7740773Z �[32;1m11:23:40�[0m |   664 |       = std::accumulate(grid_sites.begin(),
2024-02-21T11:23:40.7741574Z �[32;1m11:23:40�[0m |       |              ^~~~~~~~~~
2024-02-21T11:23:41.5824918Z �[32;1m11:23:41�[0m | make[2]: *** [src/dpl/CMakeFiles/dpl_lib.dir/build.make:76: src/dpl/CMakeFiles/dpl_lib.dir/src/Opendp.cpp.o] Error 1
2024-02-21T11:23:41.5826677Z �[32;1m11:23:41�[0m | make[1]: *** [CMakeFiles/Makefile2:2673: src/dpl/CMakeFiles/dpl_lib.dir/all] Error 2
2024-02-21T11:23:41.9383108Z �[32;1m11:23:41�[0m | make[1]: *** Waiting for unfinished jobs....

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

The ortools patch problem seems fixed, now we're seeing:

py37:

11:23:40 | /root/conda-eda/conda-eda/workdir/conda-env/conda-bld/openroad_1708513964138/work/src/dpl/include/dpl/Opendp.h: In member function 'int dpl::GridInfo::getSitesTotalHeight() const':
  11:23:40 | /root/conda-eda/conda-eda/workdir/conda-env/conda-bld/openroad_1708513964138/work/src/dpl/include/dpl/Opendp.h:230:17: error: 'accumulate' is not a member of 'std'

py38 and py310:

/root/conda-eda/conda-eda/workdir/conda-env/conda-bld/openroad_1708513921126/work/src/gui/src/layoutTabs.cpp:113:11: error: 'qOverload' was not declared in this scope
  11:22:30 |   113 |           qOverload<const Selected&>(&LayoutViewer::addSelected),

The accumulate error I also saw on my runner, and can be fixed by adding #include <numeric>. Not sure about the qOverload one because I haven't seen that before.

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

@ubfx ubfx changed the title OpenROAD: Update disable-cluster-flops-cmd.patch OpenROAD: Fix package Feb 21, 2024
@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

still getting:

2024-02-21T11:59:28.9049370Z �[32;1m11:59:28�[0m | /root/conda-eda/conda-eda/workdir/conda-env/conda-bld/openroad_1708516137315/work/src/gui/src/layoutTabs.cpp:113:11: error: 'qOverload' was not declared in this scope
2024-02-21T11:59:28.9051137Z �[32;1m11:59:28�[0m |   113 |           qOverload<const Selected&>(&LayoutViewer::addSelected),

for python > 3.7 . but openroad-linux-py37 seems to be passing! \o/
https://github.com/hdl/conda-eda/actions/runs/7988449270/job/21813113707?pr=366

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

maybe missing include or diverging CXX_STANDARD version?

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

maybe missing include or diverging CXX_STANDARD version?

Seems like qOverload was introduced in Qt 5.7, but the py37 build runs on 5.6.8. So there is probably a switch somewhere which disables qOverload for the py37 build, explaining why it doesn't see this error. For the 3.8 and 3.10 builds, building with newer Qt versions, we might have to #include <QtGlobal>. I'll give that a try.

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

For the 3.8 and 3.10 builds, building with newer Qt versions, we might have to #include . I'll give that a try.

should we pin the qt version to 5.6.8?

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

Looking at the logs it looks like for py38 and py310 the version of qt that gets resolved is older!

2024-02-21T13:22:37.0321311Z �[32;1m13:22:37�[0m |     - qt 5.6.3 h8bf5577_3
2024-02-21T13:22:50.5482877Z �[32;1m13:22:50�[0m |     - qt 5.6.3 h8bf5577_3

while py37 resolves to:

2024-02-21T13:22:26.3106916Z �[32;1m13:22:26�[0m |     - qt 5.9.7 h5867ecd_1

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

it looks like for py38 and py310 the version of qt that gets resolved is older!

Ah that's probably it! Not sure where I got those version numbers from earlier. 😄
By the way - I created a PR for the numeric include issue over at The-OpenROAD-Project/OpenROAD#4696 so we will probably have to remove that patch again once that is merged.

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

all green!
image

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

@ubfx do you want me to wait for The-OpenROAD-Project/OpenROAD#4696 to land before merging?

@ubfx
Copy link
Contributor Author

ubfx commented Feb 21, 2024

@ubfx do you want me to wait for The-OpenROAD-Project/OpenROAD#4696 to land before merging?

Up to you, I can also create a new PR on here as soon as it lands.

@proppy
Copy link
Contributor

proppy commented Feb 21, 2024

Let's unblock this then, and you (or I!) can follow up with a PR removing the patches once The-OpenROAD-Project/OpenROAD#4696 lands.

Thanks a lot for working thru this together!

@proppy proppy merged commit b24ead6 into hdl:master Feb 21, 2024
67 of 86 checks passed
@ubfx ubfx deleted the fix-openroad-ortools-patch branch February 22, 2024 07:01
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 package is broken
2 participants