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

Separate repository for XML files #934

Closed
benbarkay opened this issue Jun 20, 2018 · 11 comments
Closed

Separate repository for XML files #934

benbarkay opened this issue Jun 20, 2018 · 11 comments
Labels

Comments

@benbarkay
Copy link
Contributor

Keeping the definition XML files in a separate repository would make it easier to git sub-module them rather than copying the files.

@hamishwillee
Copy link
Collaborator

@amilcarlucas @LorenzMeier Do you have an opinion on this one? Personally I think it would be nice if the DTD lived in mavlink repo.

@benbarkay
Copy link
Contributor Author

benbarkay commented Jun 25, 2018

My case:

  1. Dialect XML and DTD files do not depend on anything, nor do they depend on the rest of the sources of this repository.
  2. Software that deals with Mavlink will always need some sort of an association with these XML files, either by depending on software derived from the definition XML files or the definition XML files directly.
  3. Thus, the definition XML files are the root of this project, everything depends on them and they depend on nothing.

Separating the definition XML files imposes little to no practical implications for this repository. It simply means that instead of being a concrete part of it, it will be a submodule.

On the other hand, separating the definition XML files from this repository removes a significant nuisance for others who wish to make their own implementations that have nothing to do with the rest of the code sources of this repository. It will make it easy for these projects to use git sub-module, and will remove the dilemma between submoduling the entire Mavlink repository or copying the definition XML files. This will make it easier to fork an contribute, receive upstream updates etc..

@amilcarlucas
Copy link
Collaborator

amilcarlucas commented Jun 25, 2018

mavlink/mavlink already contains mostly .xml files.
Code gets generated by the ardupilot/pymavlink submodule.

This seams to want to create a mavlink/base repo with two submodules:

  • mavlink/mavlink_definitions
  • ardupilot/pymavlink

That is too much confusion for almost no real added value.
Every time mavlink/mavlink_definitions gets changed we would need to change mavlink/base

What I think would be a REAL improvement would be ArduPilot/pymavlink#211 and #915

Basically add proper structuring to the XML, moving information from comments (where they are not machine parseable) into proper machine parseable information. Those are REAL improvements.

@benbarkay
Copy link
Contributor Author

benbarkay commented Jun 26, 2018

No, that is a false dilemma caused by that the structure is incoherent to the actual dependency tree, there is just mavlink, and pymavlink:

pymavlink depends on mavlink_definitions (really, it's just mavlink, but let's call it that for this one...), it makes sense that it will submodule to it. You don't need base which is the glue to the two other repositories, which are not decoupled from eachother -- pymavlink is coupled to mavlink_definitions, and mavlink_definitions is independent, there is no need for a gluing or linking repository.

Also, for instance, pull request 7 in ardupilot would require me to submodule with pymavlink, for something that is completely independent of pymavlink, that xsd file should reside right next to the xml files, separately of its dependents, that is of course, if you'd like to have more software that depends on it other than pymavlink.

There is also the added benefits of being able to improve the schema independently of the current state of pymavlink. You could run forward with the schema and version it independently, and then add support for it in pymavlink. If you commit schema to this (current, mavlink) repository that pymavlink is not yet supporting, then there would be no use to that version of the repository, its code contents will not work. It makes sense that pymavlink will pull from the schema repository depending on what its codebase supports, not the other way around.

Whether you consider projects other than your own being developed to be a real improvement depends on what you care for I suppose. If you care about the market revolving around Mavlink then you should consider independent projects finding it easy to collaborate with your work to be a real improvement, otherwise, if you care for the isolated success of pymavlink and the repositories that you contribute to then perhaps properly structuring your projects isn't a good move for you.

@stale
Copy link

stale bot commented Apr 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 23, 2020
@stale
Copy link

stale bot commented Jun 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@hamishwillee
Copy link
Collaborator

@amilcarlucas I am inclined to agree with Ben above #934 (comment)

Pymavlink is just one generator that takes xml as an input, and that xml depends on the xsd. To me the xml and xsd should be together in one repo so that anyone who wants to use it can, without taking everything else.

@amilcarlucas
Copy link
Collaborator

The current schema IS part of pymavlink. It makes sure that the genarator validates the .XML files before generating the code.

If the .xsd is not part of pymavlink, and both .xsd and .xml files change outside of the pymavlink repo, then code generation whould potentialy get broken.

Feel free to do an upstream PR for pymavlink removing the .xsd file from it. But I am preety sure that the answer above is what you will get from tridge: The xsd is there to protect the generator from ill formed .xml files.

@hamishwillee
Copy link
Collaborator

@amilcarlucas I know why it is in pymavlink, but neither of these points are valid arguments. Any generator that wants to validate it's inputs needs that XSD. If the XSD and XML were co-located then any generator, including pymavlink could take them and nothing else. The only reason to keep them in pymavlink is to say "screw you" to any other generator.

I'm happy to PR them out. My only concern is how to manage the dependency - ie can this be in mavlink/mavlink/xsd/ or does it need to be in a submodule mavlink/xsd. What would be the least disruptive?

@amilcarlucas
Copy link
Collaborator

I'm for a mavlink/mavlink/xsd/
No submodule. But please ask @tridge about this.

@hamishwillee
Copy link
Collaborator

@benbarkay @amilcarlucas OK, I spoke to Tridge about this. His "take" on this is that having a more complex module structure (by splitting out the XSD) does add developer friction that is probably not worth the hassle (vs cost of having to pull in pymavlink if you don't need it).

At this point I'm going to close this as WontFix. If someone wants to argue this then the way forward would be to attend the Devcall to explore options going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants