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

Clarification on contributing process #214

Open
traversaro opened this issue May 23, 2021 · 11 comments
Open

Clarification on contributing process #214

traversaro opened this issue May 23, 2021 · 11 comments

Comments

@traversaro
Copy link
Member

traversaro commented May 23, 2021

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 ?

@diegoferigo
Copy link
Member

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.

@traversaro
Copy link
Member Author

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 #178). Thanks!

@traversaro
Copy link
Member Author

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
Copy link
Member Author

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 traversaro mentioned this issue May 24, 2021
6 tasks
@traversaro
Copy link
Member Author

Related comment of @nunoguedelha in #209 (comment) :

@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
Copy link
Member Author

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.

@nunoguedelha
Copy link
Collaborator

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?

@diegoferigo
Copy link
Member

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.

@traversaro
Copy link
Member Author

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?

@diegoferigo
Copy link
Member

If I recall images were only part of the problem:

% Export the library. It must be in slx otherwise it will not show up in
% the Simulink Library browser.

@traversaro
Copy link
Member Author

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.

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

No branches or pull requests

3 participants