Skip to content

Added cmake support & assertive comparison tester#35

Merged
xross merged 3 commits intoxmos:developfrom
ACascarino:cmake_merge
Sep 5, 2022
Merged

Added cmake support & assertive comparison tester#35
xross merged 3 commits intoxmos:developfrom
ACascarino:cmake_merge

Conversation

@ACascarino
Copy link
Copy Markdown
Contributor

Earlier work fell behind develop, so existing work has been reworked to match
the emerging style and direction of the repository.

In _build(...), specifying cmake now changes behaviour; a
key assumption is that CMakeLists.txt exists
in a superdirectory of the xe_path, and that directory also holds /bin/.

do_run_pyxsim now supports plugins of class XsiPlugin.

pyxsim.py is now guarded against XCC_EXEC_PREFIXes that do not end in /

AssertiveComparisonTester has been added;
this is used in essentially the same way as ComparisonTester, but
uses asserts rather than writing errors to stdout.
Should be more useful for pytest-based frameworks.
Personal opinion is that it's also more readable but that's by the by.

QUESTIONS:

  • If we use run_on_simulator as the main point of entry, when it runs _build there are a couple of options it doesn't pass through (clean_only, build_options, and the new cmake, binary_child, silent). Should these be passed through by popping kwargs, similarly to how build_env and do_clean are?)

Earlier work fell behind develop, so existing work has been reworked to match
the emerging style and direction of the repository.

In _build(...), specifying `cmake` now changes behaviour; a
key assumption is that CMakeLists.txt exists
in a superdirectory of the xe_path, and that directory also holds /bin/.

do_run_pyxsim now supports plugins of class XsiPlugin.

pyxsim.py is now guarded against XCC_EXEC_PREFIXes that do not end in /

AssertiveComparisonTester has been added;
this is used in essentially the same way as ComparisonTester, but
uses asserts rather than writing errors to stdout.
Should be more useful for pytest-based frameworks.
Personal opinion is that it's also more readable but that's by the by.
Comment thread lib/python/Pyxsim/__init__.py Outdated
p.join(timeout=timeout)
if p.is_alive():
assert 0
assert 0 # Why is this here? Following two lines become unreachable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs removing. Accidental commit.

xsi.register_simthread(x)
if plugins:
for plugin in plugins:
xsi.register_plugin(plugin)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just be using plugins via sim args, do we need to support plugin args?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So XsiPlugin is a class that's defined, see e.g. pyxsim.py, line 454, where an XsiLoopbackPlugin is defined that seems to replicate the Loopback plugin in xsim. This was the last piece needed to actually use it, although whether it needs to exist at all is a good question.
In /theory/ you could define Pyxsim plugins that are clocked alongside simthreads, although in practice why you would choose to make an XsiPlugin rather than a SimThread escapes me (see e.g. the Clock class in lib_i2s/i2s_master_checker.py) - they seem like they're doing the same thing, but plugins are always clocked rather than simthreads which are waited upon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, only just noticed this. I must say not sure I understand the utility of this given this whole framework is essentially for creating plugin functionality..

Copy link
Copy Markdown
Contributor

@xross xross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. But I think we should remove that erroneous asset and comment.

@xross
Copy link
Copy Markdown
Contributor

xross commented Sep 2, 2022

Answer, to the question - yes probably. I recently reinstalled run_on_simulator as main point of entry and probably didn't do a full job. Making people call build did not seem okay (the fact its a "" for one..)

@xross
Copy link
Copy Markdown
Contributor

xross commented Sep 2, 2022

Using the ComparisonChecking returning a bool just means the assert happens "further up" the code. You only want to calculate coverage etc on passing tests and checking a bool rather an catching an assert feels nicer but personal preference really (but arguably less "pythonic")

@ACascarino
Copy link
Copy Markdown
Contributor Author

ACascarino commented Sep 2, 2022

Using the ComparisonChecking returning a bool just means the assert happens "further up" the code. You only want to calculate coverage etc on passing tests and checking a bool rather an catching an assert feels nicer but personal preference really (but arguably less "pythonic")

agreed, although do we only want to calculate coverage on passing tests? I always understood coverage to mean "how much of your codebase is exercised", not necessarily "how much of your codebase meets requirements"

Answer, to the question - yes probably. I recently reinstalled run_on_simulator as main point of entry and probably didn't do a full job. Making people call build did not seem okay (the fact its a "" for one..)

also agreed - will add that in this PR then

@xross xross merged commit caa848e into xmos:develop Sep 5, 2022
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.

2 participants