-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Initial automatic dsdl compilation #236
Initial automatic dsdl compilation #236
Conversation
This is nice. As far as I understand, the key missing bit is removing the need to install the import hook manually --- it should be done automatically when PyCyphal is imported. The values for If I remember our conversation correctly, there will be no default value for |
Yes, although might I suggest to use Regarding automatically installing import hook, I was abit worried that it could cause some weird heisenbugs, because DSDLs don't have any common prefix/namespace to be isolated from other packages. However, since it's the last in the import handler chain, maybe it's not a big deal?
I suggest |
Uhm it's fine, but I thought that PATH variables usually come without the underscore? Either way is fine.
I think it should be installed by default but we may want a function for the removal of the import hook for those special cases where the default behavior is undesirable. I am strongly in favor of having the hook installed by default with an explicit opt-out because most applications will want this default behavior.
Okay. If you're free at 19:00 our time today and would like to discuss this briefly, you would be welcome to attend our resurrected weekly call. See details here: https://forum.opencyphal.org/t/weekly-dev-call/173 |
I will try to implement removal then.
Sorry, I won't be able to join. |
Removal of the import hook is a bit complicated because it is inserted into a global |
:D I mean if you think it would be better to add a remove function, then I can do that instead. However, I see it way more complicated. For example if you use multiple import hooks with different configurations then you also need to differentiate between them. Maybe generating a unique id, which is returned as a handle by Just to be clear, the way it works now is installed by default, but can be opted out with env variable. I'm trying to fix CI now and this should be ready to go if you don't have any other comments? |
I'm not sure I understand the CI error |
It's fine. I mostly care about the default case.
Seems like the stack trace is pointing to the wrong line. The error is two lines above where it says this:
Replace It passes on your machine because you are using a more recent version of Python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Please merge the master branch and resolve the conflict. The new version number should be v1.10.
-
Please drop a note about this feature to CHANGELOG.rst.
Also, can you please update this section: https://pycyphal.readthedocs.io/en/stable/pages/architecture.html#dsdl-support The source file is here: https://github.com/OpenCyphal/pycyphal/blob/master/docs/pages/architecture.rst#dsdl-support |
Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
The CI says (in a very roundabout way) that the demo app is failing to start. Here's the relevant bit:
|
Oops, the commits linked to issue, not intended. I'm still trying to fix the CI, but we can squash the commits afterwards. |
@chemicstry FYI, your progress can be accelerated significantly if you run Nox locally. There are some instructions available in the contributor's guide. Just running |
I've used other nox commands ( |
Nox test works on Windows in the CI environment, no SocketCAN needed ¯\_(ツ)_/¯ Line 67 in 65d4854
Seems like there's one remaining issue with home directory detection and then we should be good to merge this. |
🤦 that flew over my head. After some hiccups managed to run tests on Windows. However, you should update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chemicstry are we good to merge now?
The default security policy of Windows 11 where it would randomly delete your files is also somewhat reminiscent of a virus |
Yes, I think it's ready now, finally... |
This implements automatic DSDL compilation and fixes #153.
Opening this as an incomplete draft PR so see if I'm going in a correct direction.
Todo: