Skip to content
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

Allowing duplicate definitions when parsing dsdl #68

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@thirtytwobits
Copy link
Contributor

commented Aug 8, 2019

duplicate == the same thing more than once. This is okay.
redefined == the same identifer used by incompatible definitions.
this is not okay.

In complex build systems duplicate dsdl files can get pulled in from
multiple dependencies and end up in long lists of paths that are
passed into the pyuavcan dsdl compiler. This change to the dsdl
parser allows this without throwing errors but enforces that the
duplicated definitions are compatible. This elminiates the need
for special case handling in build systems to try to find the duplicates
before invoking pyuavcan and adds a new check where such complex
systems will be notified when duplicated definitions get out of
sync.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

This does not quite contradict the v0 spec, but it would be against the v1 spec unless I misunderstood the described use case. We can accept this in v0 but not v1 unless the reasoning presented in this discussion is shown to be flawed (it is related to static compatibility checking we have defined for v1): https://forum.uavcan.org/t/new-dsdl-directive-for-simple-compile-time-checks/190/16?u=pavel.kirienko

It starts with this quote:

PyDSDL prohibits the same root namespace from being defined in several different locations. Meaning that spreading definitions belonging to the same root namespace across different directories is not permitted (e.g. /foo/bar/uavcan , /baz/bar/uavcan ).

And several posts later ends here:

What is prohibited is that you can’t invoke a parser with two root namespace directories both ending with the same final path component: /foo/bar/uavcan , /abc/baz/uavcan ; because that would imply that either the same definitions are supplied twice, or members of the same root namespace are distributed across several directories. The latter is dangerous because if a parser is ever invoked on only one of those directories, it will be unable to detect possible conflicts with the other.

Documented in the specification, section 3.1.3 File hierarchy:

A directory defining a namespace will always define said namespace in its entirety, meaning that the contents of a namespace cannot be spread across different directories sharing the same name.

Is it true that you are proposing a mere v0 implementation detail and not suggesting to change that part of v1 spec?

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Is it true that you are proposing a mere v0 implementation detail and not suggesting to change that part of v1 spec?

I guess I'm proposing that we allow this in v0 and discuss it for v1. I think we should only restrict what the types resolve to not where they come from. As I mentioned in my commit, this emerged for us as we deployed UAVCAN to distinct parts of a system and then recombined these parts in integration builds. Not only does allowing the duplication simplify the ability to deal with distributed development pipelines it actually provides a really useful compile-time check that two parts of a given system are still utilizing a compatible message definition when they are integrated. For V1 this becomes more sophisticated (read: the duplicate versus redefinition logic may be more involved) but the value is the same.

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Also: If you accept this in v0 I'll drive the changes in v1.

Allowing duplicate definitions when parsing dsdl
duplicate == the same thing more than once. This is okay.
redefined == the same identifer used by incompatible definitions.
this is not okay.

In complex build systems duplicate dsdl files can get pulled in from
multiple dependencies and end up in long lists of paths that are
passed into the pyuavcan dsdl compiler. This change to the dsdl
parser allows this without throwing errors but enforces that the
duplicated definitions are compatible. This elminiates the need
for special case handling in build systems to try to find the duplicates
before invoking pyuavcan and adds a new check where such complex
systems will be notified when duplicated definitions get out of
sync.

@thirtytwobits thirtytwobits force-pushed the thirtytwobits:master branch from 840f338 to 2ff0f3e Aug 8, 2019

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I'll drive the changes in v1.

Whatever. I don't immediately see how the obvious issues with definition consistency and enforcing versioning constraints are resolved in the workflow case you described but if you think they are resolvable then sure let's discuss that (not here). When (if) starting that discussion, could you please also provide a practical example (real-life or constructed).

I should note that the underlying idea of keeping all members of a namespace in the same directory is to make types that coexist in the same environment at runtime (which is the UAVCAN bus they are used within) also coexist in the same environment at compile time (which is the root namespace directory). If you look at it this way, you'd probably see that centralized management of a namespace is desirable for ensuring wire compatibility.

@thirtytwobits thirtytwobits merged commit 865cbca into UAVCAN:master Aug 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.