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

Update Yosys version on DependencyInstaller.sh #6140

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

gudeh
Copy link
Contributor

@gudeh gudeh commented Nov 11, 2024

I was having packages issues with EQY version, it should be the same version as Yosys. The DependencyInstaller.sh was not solving the issue. After manually updating to 0.47 my EQY and Yosys versions matched, as supposed to.

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@gudeh
Copy link
Contributor Author

gudeh commented Nov 12, 2024

I noticed on secure-CI and even nightly log reports from EQY state Yosys version as 0.43, being outdated.
Although Yosys logs itself have the correct version 0.47.

I did some greps with the logs downloaded from the last successful nightly 4736 for asap7/jpeg:

image

@eder-matheus
Copy link
Collaborator

@maliberty The CI is failing for this PR with the following messages:

[2024-11-12T21:28:44.817Z] 211.2 Cloning into 'yosys'...
[2024-11-12T21:28:44.817Z] 211.3 warning: Could not find remote branch yosys-0.47 to clone.
[2024-11-12T21:28:44.817Z] 211.3 fatal: Remote branch yosys-0.47 not found in upstream origin

Looking at both yosys and eqy repositories, in fact we don't have this yosys-0.47 branch. But we also don't have the yosys-0.43, which is the branch name used previously. Do you know how it works? I think we are missing something here...

@maliberty
Copy link
Member

@povik please advise

@povik
Copy link
Contributor

povik commented Nov 13, 2024

We dropped the yosys- prefix from release tags, the current release tag is 0.47. Sorry about that, there were some reasons for the change which I don't remember now

Signed-off-by: Augusto Berndt <[email protected]>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@eder-matheus
Copy link
Collaborator

We dropped the yosys- prefix from release tags, the current release tag is 0.47. Sorry about that, there were some reasons for the change which I don't remember now

Apparently the eqy repo is using the yosys- prefix on its tags. So using 0.47 or yosys-0.47 will lead to errors in the CI.

@maliberty
Copy link
Member

There looks to be a yosys-0.47 tag - https://github.com/YosysHQ/eqy/releases/tag/yosys-0.47

Why does that lead to an error?

@eder-matheus
Copy link
Collaborator

There looks to be a yosys-0.47 tag - https://github.com/YosysHQ/eqy/releases/tag/yosys-0.47

Why does that lead to an error?

When we use the yosys-0.47 as the version name, this error is raised:

[2024-11-12T21:28:43.833Z] #8 211.2 Cloning into 'yosys'...
[2024-11-12T21:28:43.833Z] #8 211.3 warning: Could not find remote branch yosys-0.47 to clone.
[2024-11-12T21:28:43.833Z] #8 211.3 fatal: Remote branch yosys-0.47 not found in upstream origin
[2024-11-12T21:28:44.817Z] #8 ERROR: process "/bin/sh -c /tmp/DependencyInstaller.sh -common $INSTALLER_ARGS" did not complete successfully: exit code: 128

When using 0.47, this is the error:

[2024-11-13T17:04:49.992Z] #8 312.2 Cloning into 'eqy'...
[2024-11-13T17:04:49.992Z] #8 312.4 warning: Could not find remote branch 0.47 to clone.
[2024-11-13T17:04:49.992Z] #8 312.4 fatal: Remote branch 0.47 not found in upstream origin
[2024-11-13T17:04:54.310Z] #8 ERROR: process "/bin/sh -c /tmp/DependencyInstaller.sh -common $INSTALLER_ARGS" did not complete successfully: exit code: 128

It seems that we're trying to clone both eqy and yosys repos using the same version name, which will not work.

@maliberty
Copy link
Member

@povik could you put a 0.47 tag on same eqy commit as yosys-0.47?

@povik
Copy link
Contributor

povik commented Nov 13, 2024

You're not the first to ask (YosysHQ/yosys#4586). I will need to get the internal go-ahead which will take a day or two

@povik
Copy link
Contributor

povik commented Nov 14, 2024

@povik could you put a 0.47 tag on same eqy commit as yosys-0.47?

It won't happen this week that we get a consistent tag. I would say it's simple enough but it's not up to me.

@maliberty
Copy link
Member

@gudeh you can wait or split the tag for now

@eder-matheus
Copy link
Collaborator

@gudeh you can wait or split the tag for now

I would argue for spliting the tag for now, and in the new yosys update I will go back to it and fix the tag names.

Signed-off-by: Augusto Berndt <[email protected]>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@gudeh
Copy link
Contributor Author

gudeh commented Nov 19, 2024

I noticed a limitation in the current dependency installer script. Currently, it installs yosys, sby, and eqy only if they are not found in the /usr/local/ path. This means it does not update these dependencies if an older version is already installed. We might want to consider checking the installed version and updating it if it doesn't match the latest version.

I tried implementing this feature but was not successful at a first moment, so I am currently only doing the split tag for yosys and eqy/sby.

I noticed this behavior for yosys, eqy, and sby, but it could potentially occur with other dependencies as well.

@gudeh
Copy link
Contributor Author

gudeh commented Nov 22, 2024

I do not understand why the pr-head is failing, are we still having CI issues?

12:27:33  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/mocs_compilation.cpp:8:
12:27:33  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/UVLADIE3JM/moc_findDialog.cpp:10:
12:27:33  /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/UVLADIE3JM/../../../../../src/gui/src/findDialog.h:46:8: warning: 'accept' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
12:27:33    void accept();
12:27:33         ^
12:27:33  /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qdialog.h:97:18: note: overridden virtual function is here
12:27:33      virtual void accept();
12:27:33                   ^
12:27:33  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/mocs_compilation.cpp:8:
12:27:33  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/UVLADIE3JM/moc_findDialog.cpp:10:
12:27:33  /tmp/workspace/OpenROAD-Public_PR-6140-head/build/src/gui/gui_autogen/UVLADIE3JM/../../../../../src/gui/src/findDialog.h:47:8: warning: 'reject' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
12:27:33    void reject();
12:27:33         ^
12:27:33  /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qdialog.h:98:18: note: overridden virtual function is here
12:27:33      virtual void reject();
12:27:33                   ^
12:27:33  7 warnings generated.
12:27:35  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/gui.cpp:46:
12:27:35  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/mainWindow.h:43:
12:27:35  /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/findDialog.h:46:8: warning: 'accept' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
12:27:35    void accept();
12:27:35         ^
12:27:35  /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qdialog.h:97:18: note: overridden virtual function is here
12:27:35      virtual void accept();
12:27:35                   ^
12:27:35  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/gui.cpp:46:
12:27:35  In file included from /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/mainWindow.h:43:
12:27:35  /tmp/workspace/OpenROAD-Public_PR-6140-head/src/gui/src/findDialog.h:47:8: warning: 'reject' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
12:27:35    void reject();
12:27:35         ^
12:27:35  /usr/include/x86_64-linux-gnu/qt5/QtWidgets/qdialog.h:98:18: note: overridden virtual function is here
12:27:35      virtual void reject();
12:27:35                   ^
12:27:36  2 warnings generated.
12:27:36  2 warnings generated.
12:27:37  2 warnings generated.
12:27:38  2 warnings generated.
12:27:41  [ 79%] Linking CXX static library gui.a
12:27:41  [ 79%] Built target gui
12:27:41  gmake: *** [Makefile:146: all] Error 2
12:27:41  
12:27:41  real	1m41.456s
12:27:41  user	46m10.424s
12:27:41  sys	3m20.033s

I tried restarting the run a couple of times already

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@maliberty maliberty merged commit 62c4360 into The-OpenROAD-Project:master Nov 22, 2024
11 checks passed
@gudeh gudeh deleted the patch-1 branch November 22, 2024 17:15
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