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

🧪 [linty] initial setup (preliminary) #1029

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from
Draft

🧪 [linty] initial setup (preliminary) #1029

wants to merge 45 commits into from

Conversation

stnolting
Copy link
Owner

This is a local recreation of #1027 by @racodond.

@stnolting stnolting added enhancement New feature or request CI Continuous integration-related experimental Experimental feature labels Sep 20, 2024
@stnolting stnolting self-assigned this Sep 20, 2024
@stnolting stnolting mentioned this pull request Sep 20, 2024
@stnolting stnolting changed the title [linty] initial setup 🧪 [linty] initial setup (preliminary) Sep 20, 2024
@stnolting stnolting linked an issue Sep 20, 2024 that may be closed by this pull request
sona-project.properties Outdated Show resolved Hide resolved
@stnolting
Copy link
Owner Author

Looks good now! 🚀
Thanks for your help!

Now I will take a closer look at the "Quality Profiles".

@racodond
Copy link

Looks good now! 🚀 Thanks for your help!

Now I will take a closer look at the "Quality Profiles".

You're welcome!

@stnolting
Copy link
Owner Author

Hey @racodond.

Some questions...

I do not understand the configuration of the hierarchy. How to set a specific file as top module to?

I have problems understanding the "Only use allowed libraries: ieee.std_logic_1164, work" rule. There is just a not-allowed list but no allowed list. Does that mean that everything that is not on the list is allowed?

There are a lot of warnings regarding non-synthesizeable code elements. But this is just fine as they are used in the testbenches only. How do I exclude them from the synthesis-relevant checks?

Regarding the sonar-project.properties file: the web IDE seems to provide options for sonar.hdl.file.simulationPaths and sonar.hdl.topModule so those could be removed from that file, right?

@racodond
Copy link

Hey @stnolting,

I do not understand the configuration of the hierarchy. How to set a specific file as top module to?

You do not specify a file name but an entity name.
The top module should be set in two locations:

I have problems understanding the "Only use allowed libraries: ieee.std_logic_1164, work" rule. There is just a not-allowed list but no allowed list. Does that mean that everything that is not on the list is allowed?

Yes, everything that is not listed is not allowed.
You can either work by blacklisting or whitelisting according to your needs. Either activate https://oss.linty-services.com/coding_rules?open=vhdl%3AVHDL153&rule_key=vhdl%3AVHDL153 or https://oss.linty-services.com/coding_rules?open=vhdl%3AVHDL312&rule_key=vhdl%3AVHDL312
Let me know if those two rules fulfill your needs or you'd like something else.

There are a lot of warnings regarding non-synthesizeable code elements. But this is just fine as they are used in the testbenches only. How do I exclude them from the synthesis-relevant checks?

Rules are tagged with 'simulation' or 'synthesis' or both depending on which context they apply.
https://oss.linty-services.com/coding_rules?languages=vhdl&q=synthe&open=vhdl%3AVHDL183 is tagged 'synthesis' only. So, if sonar.hdl.file.simulationPaths property at https://oss.linty-services.com/project/settings?id=neorv32&category=hdl is properly configured, you should not get issues on testbenches.
Note that properties overridden in sonar-project.properties take precedence. You do not have the same value in this file: https://github.com/stnolting/neorv32/pull/1029/files#diff-43ed9d31bea2a6d518d69836bcd1a8e6bd81bf4df96c4745792c220ca5aa549cR3
As it is a pattern, the value in the file is not correct. You should remove it and property defined through the web interface will apply.

Regarding the sonar-project.properties file: the web IDE seems to provide options for sonar.hdl.file.simulationPaths and sonar.hdl.topModule so those could be removed from that file, right?

Yes, properties should be defined through the web interface when possible.
Because when you use Linty in VS Code, properties are read from the server, not the sonar-project.properties file.

Hope it helps!

@stnolting
Copy link
Owner Author

You do not specify a file name but an entity name. The top module should be set in two locations:

Right, my fault.
I would like to use the neorv32_top as top entity. This is set in the web interface (sonar.hdl.topModule) and also in linty_hierarchy.ys. So this should be fine, right? But what is the purpose of verific -L neorv32 -vhdl ... in linty_read.ys? Is there a command reference or verific somewhere?

Yes, everything that is not listed is not allowed. You can either work by blacklisting or whitelisting according to your needs.

Ah OK! I didn't there there are two rules - I only saw the not-allowed one.

Rules are tagged with 'simulation' or 'synthesis' or both depending on which context they apply.

👍

As it is a pattern, the value in the file is not correct. You should remove it and property defined through the web interface will apply.

Done.

Yes, properties should be defined through the web interface when possible.
Because when you use Linty in VS Code, properties are read from the server, not the sonar-project.properties file.

Sounds good!

Hope it helps!

Absolutely! Thank you very much.

@racodond
Copy link

I would like to use the neorv32_top as top entity. This is set in the web interface (sonar.hdl.topModule) and also in linty_hierarchy.ys. So this should be fine, right?

Right

But what is the purpose of verific -L neorv32 -vhdl ... in linty_read.ys?

Actually, -L neorv32 is not necessary and you can remove it.

Is there a command reference or verific somewhere?

Here's the doc: https://yosyshq.readthedocs.io/projects/yosys/en/latest/cmd/verific.html

@stnolting
Copy link
Owner Author

Actually, -L neorv32 is not necessary and you can remove it.

👍

Here's the doc: https://yosyshq.readthedocs.io/projects/yosys/en/latest/cmd/verific.html

Thanks a lot!

There is still a problem with hierarchy elaboration:

[BugFinder] Hierarchy of your IP cannot be built.

sonar.hdl.topModule in the web interface is neorv32_top. linty_hierarchy.ys is hierarchy -top neorv32_top. What else is missing? 🤔

@racodond
Copy link

[BugFinder] Hierarchy of your IP cannot be built.

Logs should be saved in the action through: https://github.com/stnolting/neorv32/actions/runs/10971399621/workflow#L40
But it looks like they cannot be saved.
I'm not in front of my computer. So, could you please check if the .linty directory is created at the root of your directory and why it is not saved.

Once the workflow is fixed, the log of the elaboration of your design by Tabby CAD is available at .linty/bugfinder/logs/output.log.
Then, we'll know exactly why elaboration fails.

@stnolting
Copy link
Owner Author

stnolting commented Oct 12, 2024

I think this is quite ready. I'm very happy that all the modifications from this PR ended up in just a single file (the actual GitHub workflow file). 🚀

I'll try to adjust some of the rules before merging this (I do not want to start with a failed rating 😅). Here is the badge that we could add to the front page's status section:

Quality Gate Status

While scrolling through the reports I found some strange reliability warning: "Remove forbidden operator "mod"."

grafik

This makes sense. mod is no good for synthesis. But in this case we are working with loop/generate variables that are just constants at the end of the day (or at the end of synthesis). There is also a note regarding this:

grafik

Why is this a false positive? Did I miss something? I mean the right operand is a constant, right? Can't the checker identify this as such?

@racodond
Copy link

I think this is quite ready. I'm very happy that all the modifications from this PR ended up in just a single file (the actual GitHub workflow file). 🚀

Good!

I'll try to adjust some of the rules before merging this (I do not want to start with a failed rating 😅).

Sure

Here is the badge that we could add to the front page's status section:

OK

Why is this a false positive? Did I miss something? I mean the right operand is a constant, right? Can't the checker identify this as such?

mod operator was not in the list of allowed operators when right operand is a constant.
I made the description of the rule a bit more clearer. Let me know what you think and if it would suit your needs this way:
image

@stnolting
Copy link
Owner Author

mod operator was not in the list of allowed operators when right operand is a constant.
I made the description of the rule a bit more clearer. Let me know what you think and if it would suit your needs this way:

I still don't get it 😅

In this case the right operand IS a constant and it IS a power of two as well. I cannot see any rule options to mark individual operators as valid when the previously mentioned conditions are fulfilled.

grafik

@racodond
Copy link

I still don't get it 😅

I fixed the rule to not raise an issue when mod has a right operand that is constant and power of 2. It'll be available in the next release. I also updated the rule description to make clear what the rule is checking.

@stnolting
Copy link
Owner Author

Ahh ok 👍
Sorry, then I just misunderstood you comment. 🙈

README.md Outdated Show resolved Hide resolved
@stnolting
Copy link
Owner Author

It'll be available in the next release.

Is there already a date for it?

@racodond
Copy link

It'll be available in the next release.

Is there already a date for it?

Our goal is for mid-November.

@stnolting
Copy link
Owner Author

stnolting commented Oct 18, 2024

Hey @racodond!

How can I modify the quality gate requirements? For me, it seems like they're fixed and cannot be altered at all. 🤔 However, I would like to relax them so we do not have a "failed" status until the mod issue is fixed in the next release.

@racodond
Copy link

How can I modify the quality gate requirements? For me, it seems like they're fixed and cannot be altered at all. 🤔

I created a dedicated quality gate for your project: https://oss.linty-services.com/quality_gates/show/NEORV32
Feel free to update the requirements.

However, I would like to relax them so we do not have a "failed" status until the mod issue is fixed in the next release.

You should flag those issues as false positives.
See Bulk Change button.

image

image

@stnolting
Copy link
Owner Author

I think we are ready to merge! 🚀

I will further investigate on the quality gates and design rules from the main branch - some failing rules are acceptable for now as this is still something like "work in progress" 😉

@stnolting
Copy link
Owner Author

stnolting commented Nov 2, 2024

Oh wait, what happened here??

grafik

I did not change the token and it did not exceed the GitHub timeout 🤔

@racodond
Copy link

racodond commented Nov 4, 2024

I think we are ready to merge! 🚀

I will further investigate on the quality gates and design rules from the main branch - some failing rules are acceptable for now as this is still something like "work in progress" 😉

Good news!

@racodond
Copy link

racodond commented Nov 4, 2024

Oh wait, what happened here??
I did not change the token and it did not exceed the GitHub timeout 🤔

Did you generate a token with some expiration date?
Could you regenerate a token and try again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration-related enhancement New feature or request experimental Experimental feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check code quality in a continuous way
2 participants