dalance / sv-parser

SystemVerilog parser library fully compliant with IEEE 1800-2017
Other
396 stars 52 forks source link

Bug: sv-parser doesn't handle sources which are symlink. #86

Open Graian opened 1 year ago

Graian commented 1 year ago

While building rust source with the rules_rust(rust_library or rust_binary) build rule in the bazel build system, I encountered a bug. Bazel creates working directories using symlinks to files, but sv-parser checks if it's a file and ignores it if it's a symlink. so, requires an update to allow sv-parser to handle symlinks.

This line cause this issue.

Graian commented 1 year ago

When I modified the above-mentioned line of code to the following, I found that it fixed the problem. I don't know what side-effects this fix has on the overall behaviour of sv-parser.

if entry.file_type().is_file() ->if entry.path().is_file()

DaveMcEwan commented 1 year ago

@Graian That modification looks sensible to me. There shouldn't be any unwanted side-effects. Perhaps you could open a PR so that it's easy to merge?

I'd like to understand more about the issue because it likely affects the related project svlint (here and here).

  1. What are you using Bazel for? Instead of Cargo? Apologies if this is a silly question!
  2. How can I (or others) reproduce your issue?
  3. Is Bazel compatibility important enough to add CI checks?
Graian commented 1 year ago

@DaveMcEwan , Thanks for your comment. I have no permission to PR to dalance/sv-parser repository. and, here are my answers for your questions.

  1. What are you using Bazel for? Instead of Cargo? Apologies if this is a silly question!

    • In my development environment, I'm using bazel as my build system, so I'm trying to build an existing Cargo project on bazel. The basic external dependency is on Cargo.toml, and bazel leverages Cargo.toml and the Cargo.lock file.
  2. How can I (or others) reproduce your issue?

  3. Is Bazel compatibility important enough to add CI checks?

    • As far as I know, I haven't had any major problems with it other than the symlink issue. However, It would be nice to see it added to CI checks.
Graian commented 1 year ago

@DaveMcEwan , I've forked this repository and create a merge request!

DaveMcEwan commented 8 months ago

@Graian Would you mind closing this issue as it looks like your fix is merged.