wb-toolbox icon indicating copy to clipboard operation
wb-toolbox copied to clipboard

Clarification on contributing process

Open traversaro opened this issue 3 years ago • 11 comments

I have a doubt on how to work (especially with multiple developers/contributors) on WB-Toolbox. The repo contains two Simulink models:

  • wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl: This is the file that is meant to be modified by developers
  • wb-toolbox/matlab/library/exported/WBToolboxLibrary.slx: This is the file that is automatically generated by the export_libraries target, and it is exported in slx format, version 2014b .

My doubt is: in which format/version wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl should be stored? Before https://github.com/robotology/wb-toolbox/pull/211/files it seem that it was in format 2019b, but the last PR changed it back to 2014b format.

Any idea @diegoferigo @nunoguedelha @CarlottaSartore @Giulero @gabrielenava ?

traversaro avatar May 23 '21 18:05 traversaro

AFAIK, we never had a real guideline. Regading the mdl, that is the version used to develop the library and from which the "exported" slx is generated, was using whatever Matlab version the main developer had (for long time, mine xD). Since I was the only one accessing that file, I was not bothering to export the file for older versions.

This worked for long time due to how the maintainership was done, though now if we want we can discuss a more structured approach that allows everyone to contribute while providing some flexibility.

diegoferigo avatar May 23 '21 18:05 diegoferigo

Ack, so we just need to agree the minimum version among the people modifying it (in the next future, I guess me, @nunoguedelha and @Yeshasvitvs for https://github.com/robotology/wb-toolbox/pull/178). Thanks!

traversaro avatar May 23 '21 19:05 traversaro

Agreeing on the minimum version may not be enough, as the process of editing the wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl file while keeping it at an old version is quite cumbersome, as you can't simply use the "save" option of Simulink, but you need to Export Model to Previous Version , and you then need to use a different name for the file. If we all are on 2021a, probably we can just starting keeping the .mdl model in that format, while continuing to export the .slx in R2014b format?

traversaro avatar May 24 '21 09:05 traversaro

The PR https://github.com/robotology/wb-toolbox/pull/209/files contains the wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl model in R2021a format. If there are no objections, I would merge it as it is, and if future contributors want to use an earlier MATLAB, we can always export it back to the necessary version.

traversaro avatar May 24 '21 14:05 traversaro

Related comment of @nunoguedelha in https://github.com/robotology/wb-toolbox/pull/209#issuecomment-847159822 :

@traversaro , I didn't have a chance to review the last version. It's ok because there are two other reviewers, but actually the model file .../robotology-superbuild/src/WBToolbox/matlab/library/WBToolboxLibrary_repository.mdl is still the version 2021b and I would have noticed that before the merge 😄 . Could you please convert it to 2019b? I don't know if you want to apply the policy you mentioned recently (support the last 4 MATLAB versions)?

That was indeed my doubt, but given that we already store a version compatible with old Simulink in wb-toolbox/matlab/library/exported/WBToolboxLibrary.slx, what's the point of keeping another one wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl?

traversaro avatar May 24 '21 19:05 traversaro

Agreeing on the minimum version may not be enough, as the process of editing the wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl file while keeping it at an old version is quite cumbersome, as you can't simply use the "save" option of Simulink, but you need to Export Model to Previous Version , and you then need to use a different name for the file. If we all are on 2021a, probably we can just starting keeping the .mdl model in that format, while continuing to export the .slx in R2014b format?

Not sure if @nunoguedelha has some input n this point.

traversaro avatar May 24 '21 19:05 traversaro

Sorry, I do indeed. As far as I understood, the rational behind the existence of the two library formats .mdl and .xls was as follows: if a developer wants to change the library, he should change the .mdl then export it to .xls using the script export_library.m. I guess this approach was to make sure that the library is always properly configured (EnableLBRepository set to on, version R2014b, etc). I personally don't see the point in keeping the .mdl at all, in any version, for two reasons:

  • Probably the initial motivation to keep an .mdl file was to be able to merge it manually, relying on the text content, i.e. without the MATLAB dedicated tools (visdiff, etc). Considering the danger of merging this way, and our growing experience with the MATLAB diff/merge tools, I think this motivation has become obsolete.
  • Keeping the .mdl, specially in a more recent version than R2014b doesn't reduce the effort of exporting it to R2014b. And waiting that a user having MATLAB version < current .mdl version, for istance R2019b < R2021b, needs it before doing the conversion, is problematic. He has to:
    1. report the issue and ask someone (X) with MATLAB R2021b to kindly do the conversion.
    2. wait for that X to catch the request
    3. wait for X to be available for doing the conversion and push the changes.

Even if X is extremely responsive, I don't think that's a reliable way to proceed, and just increases the overall effort.

A simple way to go would be to

  1. Keep only the .xls (unless I missed some other motivation).
  2. change the script export_library.m, integrating the steps to export the library to version R2014b from the same input file (copy to a different name, load it, export to R2014b as the original name, close).

What do you think guys?

nunoguedelha avatar May 24 '21 20:05 nunoguedelha

If it would be my choice, i.e. if I had to maintain the library, I'd probably keep the current structure. Working with a textual file is too handy in my opinion, and this could be even more helpful when there are many collaborators and many PR that have to merged together. Now, if this is a shared opinion among the current maintainers, I wouldn't have anything against moving towards a single slx library.

diegoferigo avatar May 25 '21 07:05 diegoferigo

And just to double check, the reason why we don't have a single .mdl file is that images do not work with mdl files, if I remember correctly?

traversaro avatar May 25 '21 07:05 traversaro

If I recall images were only part of the problem:

https://github.com/robotology/wb-toolbox/blob/4629ec927eafb0b0d8ff2cc7cbe2ef2f431c7cd6/matlab/export_library.m#L27-L28

diegoferigo avatar May 25 '21 07:05 diegoferigo

We discussed on this with @nunoguedelha . We agreed that the process of manually exporting to R2014b (or any other version that is the one of the running MATLAB) is quite error prone, so just relying on editing and re-exporting wb-toolbox/matlab/library/exported/WBToolboxLibrary.slx is not a desirable workflow. On the other hand, even if we keep wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl in the repo to an old version, we would have exactly the same problem that we have with wb-toolbox/matlab/library/exported/WBToolboxLibrary.slx, so that is not a solution as well, unless we all coordinate to use exactly the same MATLAB version, that may not be feasible in practice (even for conflicting requirements across projects and teams).

As a compromise, we reached the following agreement:

  • We delete the wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl file from the committed repo, to have only a single source of truth given by wb-toolbox/matlab/library/exported/WBToolboxLibrary.slx
  • We add a script (and associated build target) import_library, that creates on the fly the wb-toolbox/matlab/library/WBToolboxLibrary_repository.mdl model for the version of MATLAB used by the contributor.

In this way, the contribution workflow remains similar to the existing one, with the only difference that the contributor needs to run import_library as first step before contributing.

traversaro avatar May 27 '21 11:05 traversaro