ToolboxHub / ToolboxToolbox

Declarative dependency management for Matlab.
The Unlicense
27 stars 11 forks source link

Handling of old registry format #104

Closed githubkusi closed 4 years ago

githubkusi commented 4 years ago

PRQ #94 introduced the record field toolboxSubfolder. A previous registry record (probably saved in the prefs) doesn't have that field. Provide the user with a proper error message and a solution

The code still breaks if the user has an old record in his prefs (which happened with a colleague of mine), but at least it is more clear now. We could try to handle this correctly (being backward-compatible), but I don't think it's worth the risk & time. Or what do you think?

The best solution for future changes would be to introduce a version field in the record generated by tbToolboxRecord().

githubkusi commented 4 years ago

please merge first PR #103, otherwise the wrong commits are assigned to THIS pull request

DavidBrainard commented 4 years ago

I'm a little confused here. Many toolbox records do not have the toolboxSubfolder field, and yet they do not seem to be breaking. What is the exact situation that causes the failure?

DavidBrainard commented 4 years ago

Nope, this PR doesn't fix the TbGitAndSanityTest suite - four still broken on my machine, but as noted elsewhere these were not broken for me prior to PR 102. Will try to dig in a little.

DavidBrainard commented 4 years ago

The (or at least, one) issue occurs in the call to parser.parse in tbParsePrefs, as called from tbAddToolbox -> tbWriteConfig -> tbParsePrefs. The value of persistantPrefs is empty, and this makes the parser unhappy when called with arg (varagin{:}) because there is no key associated with empty variable persistantPrefs.

I have not delved into how the new persistantPrefs is implemented, but maybe the problem will be immediately obvious to you?

githubkusi commented 4 years ago

that's strange. So you say getpref('ToolboxToolbox') is empty for you? Even then, an empty persistent pref struct does not get fed into parser.parse. So I don't understand, I'm sorry. Could you please post a callstack?

DavidBrainard commented 4 years ago

runtests TbGitAndSanityTest/testAddOneAtATime:

================================================================================ Error occurred in TbGitAndSanityTest/testAddOneAtATime and it did not run to completion.

Error ID:
---------
'MATLAB:InputParser:ParamMustBeChar'
--------------
Error Details:
--------------
Error using tbParsePrefs (line 74)
Expected a string scalar or character vector for the parameter name.

Error in tbWriteConfig (line 18)
prefs = tbParsePrefs(tbGetPersistentPrefs, varargin{:});

Error in tbAddToolbox (line 51)
tbWriteConfig(config, persistentPrefs, prefs);

Error in TbGitAndSanityTest/testAddOneAtATime (line 77)
                results = tbAddToolbox(tbGetPersistentPrefs, ...
DavidBrainard commented 4 years ago

And indeed, getpref('ToolboxToolbox') is empty. I run the TbTb in its default configuration, so I don't think any preferences ever get set.

DavidBrainard commented 4 years ago

I don't think my diagnosis above is quite right. What is true is that it errors as in the call stack above, and that persistentPrefs is empty.

githubkusi commented 4 years ago

Indeed, if setpref('ToolboxToolbox') is empty, it also fails here: You see the same failures when running all tests on TbGitAndSanityTest?

Failure Summary:

 Name                                      Failed  Incomplete  Reason(s)
=========================================================================
 TbGitAndSanityTest/testAddOneAtATime        X         X       Errored.
-------------------------------------------------------------------------
 TbGitAndSanityTest/testSubfolderPath        X         X       Errored.
-------------------------------------------------------------------------
 TbGitAndSanityTest/testTwoSubfolderPaths    X         X       Errored.
-------------------------------------------------------------------------
 TbGitAndSanityTest/testHook                 X         X       Errored.
DavidBrainard commented 4 years ago

Yes, I'm pretty sure it was those four (I am at another machine now, but there were definitely four failures from TbGitAndSanityTest, and only four failures.)

githubkusi commented 4 years ago

should be fixed with #105