-
Notifications
You must be signed in to change notification settings - Fork 25
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
ci: combine workflows and homogenize testing target names; limit concurrency #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@umarcor Great job!
- The
makefile
now looks cleaner. - The
Tests.yml
is doing the same jobs as the separate files was. - I'm not sure if removing comments was a good choice. it would be easier for other developers to understand what is written if they were there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohanad0mohamed Could we create a new PR to split the main DRC to smaller parts as well? |
@atorkmabrains There is already an issue about that request: #15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes seem great. And I approve changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two very minor comments.
4de79db
to
88b10ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@umarcor You moved the klayout version to klayout 0.27.10 we want to use klayout 0.27.8.
@atorkmabrains I removed the commit bumping klayout. |
Signed-off-by: Unai Martinez-Corral <umartinezcorral@antmicro.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Will merge if CI goes green.
|
||
name: Regression testing | ||
|
||
# https://docs.github.com/en/actions/using-jobs/using-concurrency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than linking to the concurrency documentation, can you explain why we want / need the concurrency statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 14838d3.
Signed-off-by: Unai Martinez-Corral <umartinezcorral@antmicro.com>
Signed-off-by: Unai Martinez-Corral <umartinezcorral@antmicro.com>
Updating primitives names in cdl files used for LVS testing
Fixes #13
/cc @mithro