String as define issue

[vborchsh]

String as define doesn't work. At least, for Questa and Icarus. I tried some tricks, but no luck. Is there a way to do it?

Thanks.

[ktbarrett]

All failing tests are due to syntax error.

[vborchsh]

Ah, sorry, I was not clear. And I have missed commas :)

All failing tests are due to syntax error.

This is true, because of wrong formatting of string define parameter for the simulators. E.g:

iverilog ...blabla... -DDEFINE_PATH=path/to/some/file.wow

This part must be like -DDEFINE_PATH="path/to/some/file.wow"

The same situation for Questa:

vlog ...blabla... +define+DEFINE_PATH=path/to/some/file.wow

to +define+DEFINE_PATH="path/to/some/file.wow"

I played a bit with combinations of lists, tuples, strings and so on. But no luck. Maybe there is a way to do it...

[ktbarrett]

I think the real answer is to create a function that takes the value and serializes it appropriately and use that instead of as_tcl_value.

[themperek]

Sorry I approve by mistake.

[themperek]

Would this work "DEFINE_PATH": "\"path/to/some/file.wow\"" ?

approved by accident -- still outstanding issues to be resolved

[Lockedloop]

Removing the as_tcl_value completely works for me.

    @staticmethod
    def _get_define_options(defines: Mapping[str, object]) -> Command:
        return [
            f"+define+{name}={value}"
            for name, value in defines.items()
        ]

I could be missing something, but seeing that _get_parameter_options does the same thing in the current version, it seems to me as a quick fix. Does anyone know why was the as_tcl_value introduced in the first place?

[ktbarrett]

@Lockedloop Good to know. as_tcl_value was added to serialize values in expected TCL formatting since some simulators need that, but it's obviously not appropriate here. I'll try to get around to finishing this PR off if @vborchsh doesn't in the next couple days.

[Lockedloop]

@Lockedloop Good to know. as_tcl_value was added to serialize values in expected TCL formatting since some simulators need that, but it's obviously not appropriate here. I'll try to get around to finishing this PR off if @vborchsh doesn't in the next couple days.

Great, thanks a million!

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8e189cb) 66.89% compared to head (1ae7c6e) 66.40%.
Report is 9 commits behind head on master.

Files	Patch %	Lines
src/cocotb/runner.py	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3391      +/-   ##
==========================================
- Coverage   66.89%   66.40%   -0.49%     
==========================================
  Files          49       49              
  Lines        8457     8462       +5     
  Branches     2384     2388       +4     
==========================================
- Hits         5657     5619      -38     
- Misses       1689     1719      +30     
- Partials     1111     1124      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vborchsh]

@Lockedloop Good to know. as_tcl_value was added to serialize values in expected TCL formatting since some simulators need that, but it's obviously not appropriate here. I'll try to get around to finishing this PR off if @vborchsh doesn't in the next couple days.

Sorry, just found this thread. Completely fine for me if you finish this :)

[ktbarrett]

Not sure what the Riviera and Xcelium issues are, but the icarus ones are because the one on apt is too old, I'll have to split that into a separate test and put a version ifdef on it. But I can see it being done soon.

[cmarqu]

Xcelium appears to need more/different quoting. Maybe one that as_tcl_value provides.

[ktbarrett]

Using as_tcl_value didn't help. The issue appears to be that the slashes need escaping.

[ktbarrett]

\\\"But\\ of\\ course!\\ It\\ needs\\ to\\ be\\ +define+DEFINE_PATH=\\\"path/to/some\\\\\\\"(random\\ quote)/file.wow\\\"\\\"

[ktbarrett]

So Xcelium doesn't seem to support spaces in defines passed on the command line no matter how many backslahes I add. This must be a limitation of their option parser. And it doesn't seem to like $error()? I'll just leave the problem child be.

And I thought I had Riviera working; apparently not. I'm done with this. Someone else figure it out.

[cmarqu]

So Xcelium doesn't seem to support spaces in defines passed on the command line no matter how many backslahes I add. This must be a limitation of their option parser. And it doesn't seem to like $error()? I'll just leave the problem child be.

@imphil Do you think Cadence would help us here?

[imphil]

@imphil Do you think Cadence would help us here?

Absolutely. Can you write the text you'd like me to submit as issue to Cadence? I'll then copy-paste it over.