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

Use updated API for svlint v0.8.0 #7

Merged
merged 18 commits into from
Jul 3, 2023
Merged

Conversation

DaveMcEwan
Copy link
Contributor

@DaveMcEwan DaveMcEwan commented Jun 21, 2023

  • Add some basic test infrastructure, extending Setup basic CI flow #6.
    This PR can be merged either after or instead of Setup basic CI flow #6.
  • Superceeds Updated example for Vec of Rule #5 by supporting a list of rules which may be either TextRule or SyntaxRule.
    Thanks @skjdbg :)
  • Should not be merged until svlint v0.8.0 is released.
    After that, another commit is required to this PR: Cargo.toml dependency on svlint should be "0.8", not the pin to a specific commit in my fork.
    DONE
  • Updates to README.

DaveMcEwan and others added 16 commits June 19, 2023 17:08
There are currently no tests, but this does attempt a `cargo build`, which
is sufficient to detect API breakage between svlint and svlint-plugin-sample.
Here's the full warning:

```
warning: `extern` fn uses type `dyn Rule`, which is not FFI-safe
 --> src/lib.rs:6:35
  |
6 | pub extern "C" fn get_plugin() -> *mut dyn Rule {
  |                                   ^^^^^^^^^^^^^ not FFI-safe
  |
  = note: trait objects have no C equivalent
  = note: `#[warn(improper_ctypes_definitions)]` on by default
```

This is expected because, as rustc notes, trait objects have no C
equivalent, so should be ignored.
Preparation for updating the API to use both `TextRule` and `SyntaxRule`.
- Boilerplate should be replaced with updated version of `combine_rules`.
  This is just to try getting things working.
- Tested with local build of svlint.
@DaveMcEwan
Copy link
Contributor Author

As per dalance/svlint#253, this will be changed from draft to proper after svlint v0.8.0 is released.

@skjdbg
Copy link
Contributor

skjdbg commented Jun 21, 2023

On my local version of svlint (where I had pulled some of your changes) the SyntaxRules work fine, but the TextRule reports errors multiple lines before the actual errors.

I'm recompiling to check with your last commits, but it take a very long time on this machine. I'll tell you if anything changes.

@DaveMcEwan DaveMcEwan mentioned this pull request Jun 21, 2023
4 tasks
@skjdbg
Copy link
Contributor

skjdbg commented Jun 21, 2023

@DaveMcEwan I tested with your last commits and I still have the error. It seems to only work properly if the regex "XXX" matches on the first line of the file.
I don't see any obvious mistakes in textrules_check or in forbidden_regex and I don't see any changes in the printer so I'm not sure what could be causing this.

@DaveMcEwan
Copy link
Contributor Author

Does that mean cargo test reports basic_text_fail_2of3 and basic_text_fail_3of3 are failing? I see all 3 passing like https://github.com/DaveMcEwan/svlint-plugin-sample/actions/runs/5333285376/jobs/9663558718

@skjdbg
Copy link
Contributor

skjdbg commented Jun 22, 2023

All tests pass, yet when I call ./target/debug/svlint.exe -p ../path/to/svlint_plugin_sample.dll ./path/to/test_file.sv (content of test_file.sv at the end of this comment)

  • The first mistake is treated properly.
  • The second mistake is detected on the right line. but no text is shown
  • All other mistakes are detected, but the line at which it is detected is wrong (and so the text shown is also wrong

Here is the console output

Fail: forbidden_regex
   --> path\to\test_file.sv:1:1
  |
1 | class driver extends uvm_driver#(seq_item); // XXX
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ hint  : Remove the string 'XXX' from all lines.
  |                                                    reason: XXX is not meaningful enough.

Fail: forbidden_regex
   --> path\to\test_file.sv:2:0
  |
2 |
  |  hint  : Remove the string 'XXX' from all lines.
  |  reason: XXX is not meaningful enough.

Fail: forbidden_regex
   --> path\to\test_file.sv:3:35
  |
3 |   task run_phase (uvm_phase phase);
  |                                   ^ hint  : Remove the string 'XXX' from all lines.
  |                                     reason: XXX is not meaningful enough.

Fail: forbidden_regex
   --> path\to\test_file.sv:5:11
  |
5 |       // Test
  |           ^^^ hint  : Remove the string 'XXX' from all lines.
  |               reason: XXX is not meaningful enough.

Fail: forbidden_regex
   --> path\to\test_file.sv:7:14
  |
7 |       foo <= bar;
  |              ^^^^ hint  : Remove the string 'XXX' from all lines.
  |                   reason: XXX is not meaningful enough.

and the content of test_file.sv

class driver extends uvm_driver#(seq_item); // XXX
//XXX
  task run_phase (uvm_phase phase);
      foobar.get_next_item(XXX);
      // Test
      foo <= bar; //XXX
      foo <= bar;

      baz <= XXX;
  endtask
endclass : droiver

@DaveMcEwan
Copy link
Contributor Author

I can't reproduce this. I've just tried in a clean workspace and get this:
screenshot

The commands I'm running to reproduce:

cd /work/damc/ # A new working area.
vim test_file.sv # Copy your testcase.

# Build the plugin.
git clone https://github.com/DaveMcEwan/svlint-plugin-sample.git
pushd svlint-plugin-sample && git checkout api && cargo build && cargo test && popd

# Get the matching branch for svlint.
git clone https://github.com/DaveMcEwan/svlint.git
cd svlint && git checkout api

# Run the test.
cargo run -- -p ../svlint-plugin-sample/target/debug/libsvlint_plugin_sample.so ../test_file.sv

@DaveMcEwan
Copy link
Contributor Author

I get consistent behaviour between Linux (previous comment) and Windows.
image

@skjdbg
Copy link
Contributor

skjdbg commented Jun 22, 2023

I think it's a matter of CRLF vs LF. I repeated your steps on a linux machine, but wrote test_file.sv from vscode on windows and nothing changed.
Then I deleted test_file.sv and rewrote it manually from within a linux terminal editor, and I got your results

@DaveMcEwan
Copy link
Contributor Author

DaveMcEwan commented Jun 22, 2023

Ok cool. I can reproduce that with set ff=dos in Vim.
I think it's related to https://github.com/dalance/svlint/blob/master/src/main.rs#L278-L280,
LintFailed, and a few places in src/printer.rs that results in a mis-count of the line number.

Summary of issue: svlint reports wrong line numbers for TextRule failures in DOS-format files (CRLF).

From a quick look, it doesn't look trivial to fix! However, a fix shouldn't conflict with any of the open PRs.

@skjdbg
Copy link
Contributor

skjdbg commented Jun 22, 2023

On another note, in forbidden_regex.rs, why do you format the regex ?
As far as I know, this

let r = format!(r"XXX");
self.re = Some(Regex::new(&r).unwrap());

should be the same as this

let r = r"XXX";
self.re = Some(Regex::new(r).unwrap());

@DaveMcEwan
Copy link
Contributor Author

On another note, in forbidden_regex.rs, why do you format the regex ?

You're right, I just copied from https://github.com/dalance/svlint/blob/master/src/textrules/header_copyright.rs for the example, but it isn't necessary.

@skjdbg
Copy link
Contributor

skjdbg commented Jun 22, 2023

Well then I have no more issues. =)

DaveMcEwan added a commit to DaveMcEwan/svlint that referenced this pull request Jun 22, 2023
…kjdbg

Discussed here:
dalance/svlint-plugin-sample#7 (comment)

Reproduction command:
`SVLINT_CONFIG=doslinenum.toml cargo run -- forbidden_regex.sv`
The message printed doesn't line up with the

Notes:
- This doesn't fit into the normal testcase structure because `build.rs`
  splits testcase files into subtests, which are always ff=unix.
- `build.rs` depends on the normal testcase files existing, but they're
  empty in this commit.
- Git can be configured to change line endings, so ensure that
  `./forbidden_regex.sv` has CRLF (DOS-format) line endings.
DaveMcEwan added a commit to DaveMcEwan/svlint that referenced this pull request Jun 22, 2023
…kjdbg

Discussed here:
dalance/svlint-plugin-sample#7 (comment)

Reproduction command:
`SVLINT_CONFIG=doslinenum.toml cargo run -- forbidden_regex.sv`
The output refers to wrong lines.

Notes:
- This doesn't fit into the normal testcase structure because `build.rs`
  splits testcase files into subtests, which are always ff=unix.
- `build.rs` depends on the normal testcase files existing, but they're
  empty in this commit.
- Git can be configured to change line endings, so ensure that
  `./forbidden_regex.sv` has CRLF (DOS-format) line endings.
@DaveMcEwan
Copy link
Contributor Author

@skjdbg I put a fix for the CRLF issue into the api branch of svlint. Would you mind checking that it's fixed for you too?

@skjdbg
Copy link
Contributor

skjdbg commented Jun 22, 2023

It works on my side !

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.

3 participants