Use POSIX-style paths in #includes

[edwinbalani]

The __str__() method of a pathlib.Path is platform-dependent. We were using this when generating #include directives, but to produce host-independent C/C++ code we probably shouldn't (e.g. so that code generated on Windows can compile on a Linux system without Windows-style).

The current behaviour ultimately stems from the fact that the implementation presented at pathlib.Path depends on the interpreter host platform. These are individually available as the PosixPath and WindowsPath classes.

An alternative to this PR would be to use pathlib.PosixPath (or PurePosixPath) around the code explicitly, rather than relying on platform-dependent behaviour. However, it would probably be a much bigger diff than this tiny change. I think keeping things as platform-native Path objects would be better, for instance so that the same path variable can be used to query the real filesystem without surprises.

Closes #129.

[pavel-kirienko]

Thanks, @edwinbalani! It looks good but please bump the patch version number in version.py to let our CI/CD setup release the new version automatically after merge.

[edwinbalani]

please bump the patch version number in version.py to let our CI/CD setup release the new version automatically after merge.

Thanks - I'll do so. I think CONTRIBUTING.md could be clearer on this, since it currently only says to do this when "committing to master". In one interpretation, I'm only committing to my branch at the moment :)

Do you think there are any situations where a PR shouldn't bump the version number, and are they rarer than the ones where a contributor should? If so, then perhaps the guidance could be reworded to say you normally should bump as part of your PR changes, and outline situations where it shouldn't be done.

I ought to test this change informally on Windows to check that the behaviour's actually changed; should be able to do this today too.

[pavel-kirienko]

Do you think there are any situations where a PR shouldn't bump the version number, and are they rarer than the ones where a contributor should? If so, then perhaps the guidance could be reworded to say you normally should bump as part of your PR changes, and outline situations where it shouldn't be done.

I don't think there is a sensible scenario where you shouldn't but we should wait for @thirtytwobits to confirm.

I ought to test this change informally on Windows to check that the behaviour's actually changed; should be able to do this today too.

Speaking of which, there's #60

[edwinbalani]

It seems the changes haven't quite done the trick. Where we used to get this:

#include <nunavut\support\serialization.h>
#include <uavcan\_register\Name_1_0.h>
#include <uavcan\_register\Value_1_0.h>
#include <uavcan\time\SynchronizedTimestamp_1_0.h>
// ...

we now get

#include <nunavut/support/serialization.h>
#include <uavcan\_register\Name_1_0.h>
#include <uavcan\_register\Value_1_0.h>
#include <uavcan\time\SynchronizedTimestamp_1_0.h>
// ...

This is in the generated header for uavcan.register.Access.1.0, using nnvg -e .h .\public_regulated_data_types\uavcan, with Python 3.9.0 and a fresh clone of the types.

So I think there are more potential instances of str() that need stamping out.

[pavel-kirienko]

Adding a test that ensures that there are no \-s would help here. It is expected to be redundant on GNU/Linux but once we support Windows in the CI it will help us avoid regressions.

[edwinbalani]

...just one bad str(), which has hanging around at the bottom of the same file 😁. I've also bumped the version number, and I think this is good to go now.

I've since learnt more about nnvg, and changed how I invoke it. With nnvg -l c -I .\public_regulated_data_types\uavcan\ -O reg_gen .\public_regulated_data_types\reg I get nice-looking output:

// reg.drone.phy.acoustics.Note.0.1
#include <nunavut/support/serialization.h>
#include <uavcan/si/unit/duration/Scalar_1_0.h>
#include <uavcan/si/unit/frequency/Scalar_1_0.h>
#include <uavcan/si/unit/power/Scalar_1_0.h>
#include <stdlib.h>

[edwinbalani]

Adding a test that ensures that there are no \-s would help here.

This should be straightforward, right? (Famous last words...) We probably just need to check that lines starting with '#include' don't have a \.

[pavel-kirienko]

That's one way to handle it, yes. Maybe you can narrow the scope of the test such that it only involves the IncludeGenerator itself without having to generate the whole file, but ultimately it doesn't matter much, I think.

[thirtytwobits]

Thanks!

[edwinbalani]

I think test.gentest_filter.test_filters:test_python_filter_includes actually covers us here, by hard-asserting that the output of the IncludeGenerator exactly matches a given list, so no new test should be needed. It's only testing the C++ case, but both it and the C language support use the same IncludeGenerator, unsurpisingly.

So whenever CI on Windows happens, this should hopefully be one existing test that doesn't start failing out of the blue 😅

[thirtytwobits]

My change is blocked behind this one. @pavel-kirienko , is there more you need here?

[pavel-kirienko]

Sorry, I didn't realize that you were waiting for my review.