stage -> master for #246 (fix for #228 (Matlab / mYm improved version/compatibility support)) #258

ixcat · 2020-07-17T16:10:59Z

see also: #246

…upDJ.m - ensures earlier detection of outdated components - part of fix for #228

- setupMYM now manages actual install/setup of mym - setupDJ calls this to install mym, and checks versions. currently: - we install 'master' (other options 'X.Y.Z', maps to github releases e.g: https://github.com/datajoint/mym/archive/v2.7.2.zip) - we check for >2 or >= 2.6

fix for #228 (Matlab / mYm improved version/compatibility support)

guzman-raphael

Overall PR looks good and ready. Have minor suggestions related to style, and, since we are modifying it, returning the installed versions from setupDJ.m and setupMYM.m when there is an output value defined. Can you also add a test? Should be a pretty simple one and self contained e.g. force install an old version of mym, check version, then force install from master over it, and check version.

setupMYM.m

setupDJ.m

setupMYM.m

setupDJ.m

guzman-raphael · 2020-07-31T15:00:48Z

setupMYM.m

@@ -0,0 +1,67 @@
+function setupMYM(version, force)


Suggested change

function setupMYM(version, force)

function mymVersion = setupMYM(version, force)

Would be nice if calls to this returned the installed version. Could also make sense to apply to setupDJ.

can see returning version is useful, renaming function I'm less clear on since this doesn't simply return the version but also may modify the system

No need to rename the function but the way in Matlab that you return arguments is by defining a variable to output result to. Then you simply set that within your method. Such as mymVersion.

ah, misread this. what should we return? string? 3-array of versions?

Hmm don't remember our convention at the moment but I'd likely just stick to how version is returned in the dj.version function.

so, version support is partially implemented - some issues w/r/t proposed scheme ('how dj.version works') below -
current approach applies the input argument (string) to the corresponding file, and if mymSetup succeeds, returns this value back to the user, and replaces the INVOKED variable with INSTALLED_VERSION to cache this for subsequent non-force calls. Paused here as anything further needs wider discussions / more intrusive changes.

so - to the issues on 'how dj version works':

This function has to support new abstraction of 'master' (if I install 'master', should I get 'master' back or '2.3.4' ? if 2.3.4 where is master:2.3.4 reference mapping stored for confirmation/tests/maintenance, etc). For now, returning as specified in input argument if install succeeded (see point 2). setupDJ doesn't suffer from this problem because it doesn't need to manage other files / take any version arguments / etc

mym('version') and dj.version() both return 'struct with fields', which isn't how we are expecting the input argument, which makes version checking of input arg vs result tricky even without the master:version mapping problem in point 1

mym/mymSetup doesn't directly return anything, so the only way to get the 'real' mym version is mym('version') after calling mymSetup(), not sure if there could be issues here w/r/t some hidden mym internal state / install sequence problems.

also related to the unit test: unit test is clearing various directories but calling back into setupMYM() without force, which means unit test environment is unreliable because of INSTALLED_VERSION caching. not sure how best to handle this.

Thanks for the update. I will provide feedback on these points in my review where they associate within the code.

updates for #258 {setupDJ,setupMYM}.m: minver 9.1/2016b, adjust print/error/warnings.

…here

updates for #258 {setupDJ,setupMYM}.m: version return, test case updates

guzman-raphael · 2020-09-14T16:40:39Z

+tests/TestSetupMYM.m

@@ -0,0 +1,117 @@
+
+classdef TestSetupMYM < tests.Prep


In order for your tests to run alongside the existing tests, you will need to add an entry for this TestSetupMYM class to +tests/Main.m.

guzman-raphael · 2020-09-14T17:27:52Z

setupMYM.m

+        % note: for 'master', is currently 'master' not mym('version')
+        %   using mym('version') needs a master:version mapping,
+        %   and how to handle this is a wider dev issue (releses, tests, etc)
+        mymVersion = INSTALLED_VERSION;


For the master case, I don't think we should return master for mymVersion. It is not explicit enough what is the actual installed version. We do not necessarily need the mapping here as it is installing master at that time. If user would like to update to latest, simply calling SetupMYM('master', true) would be fine.
Also, caching the INSTALLED_VERSION seems unnecessary as we can simply ask mYm via mym('version'). Therefore, we can simply pass through mYm version for the output to this method.

The caching of INSTALLED_VERSION evolved from just caching if the function ran (persistent INVOKED), and was modeled after setupDJ for consistency. Not sure why we are caching there, but setupDJ now calls this function, so thought this was useful (assuming that caching to be implemented on purpose) to keep the same.

Definately 'cargo cult' to be sure, but if removing cache here, seems like 'why is this happening in setupDJ' should be discussed.

so basically in response to #258 (comment) people calling with 'master' would just assume successful completion is the 'correct' master?

On it's own, makes sense - wasn't sure how strict we wanted to do this and if e.g. we wanted to leave dj matlab installing 'master' but have a strict version sanity check, where to manage that, etc.

also: so basically in response to #258 (comment) people calling with 'master' would just assume successful completion is the 'correct' master?

On it's own, makes sense - wasn't sure how strict we wanted to do this and if e.g. we wanted to leave dj matlab installing 'master' but have a strict version sanity check, where to manage that, etc.

guzman-raphael · 2020-09-14T17:51:57Z

setupMYM.m

@@ -0,0 +1,76 @@
+function mymVersion = setupMYM(version, force)


After taking another look, perhaps we should change the function name to updateMYM so we do not confuse this method with mymSetup.m (from mYm).

Named this to be consistent with 'setupDJ', realized it was confusing but thought in-matlab consistency made more sense to follow since mymSetup is in another repo. Not sure what's best.

guzman-raphael · 2020-09-14T18:22:57Z

setupMYM.m

+
+    persistent INSTALLED_VERSION;
+
+    if ~isempty(INSTALLED_VERSION) && ~force


Seems like we can consolidate the overall logic a bit since some of the checks can be redundant. Think it is fair to assume if user 'forces' an install, we should always reinstall (even if the same version). Logic could look like:

base = fileparts(mfilename('fullpath')); mymdir = fullfile(base, 'mym'); if isdir(mymdir) && ~force error('mYm already installed, etc.'); mymVersion = []; return; elseif isdir(mymdir) && force warning('mYm detected but forcing update, etc.'); rmdir(mymdir, 's'); end {{install mYm}} mymVersion = mym('version');

guzman-raphael · 2020-09-14T18:52:21Z

setupMYM.m

+        fprintf('downloading %s to %s\n', mymURL, target);
+        target = websave(target, mymURL);
+
+        extdir = fullfile(base, sprintf('mym-%s', version));


This might not select the dir properly if vX.X.X.

^ see above - versions as written should be either names or 'x.y.z' strings ( goes to overall question of how to represent abstract vs real versions & where to store this). Not saying this is how it should be, but, that's the API model currently.

guzman-raphael · 2020-09-14T19:08:48Z

+tests/TestSetupMYM.m

+
+            % TODO: how manage version string?
+            ms = setupMYM('2.7.2');
+            testCase.verifyTrue(strcmp(ms, '2.7.2'));


Suggested change

testCase.verifyTrue(strcmp(ms, '2.7.2'));

testCase.verifyTrue(strcmp([ms.major '.' ms.minor '.' ms.bugfix], '2.7.2'));

guzman-raphael · 2020-09-14T19:09:59Z

+tests/TestSetupMYM.m

+            [dir, nam, ext] = fileparts(mfilename('fullpath'));
+            tmym = strcat(dir, '/../', 'mym');
+
+            if isdir(tmym)
+                fprintf('testDefaultInstall: mymdir %s exists. removing\n', ...
+                        tmym);
+                rmdir(tmym, 's');
+            end


Suggested change

[dir, nam, ext] = fileparts(mfilename('fullpath'));

tmym = strcat(dir, '/../', 'mym');

if isdir(tmym)

fprintf('testDefaultInstall: mymdir %s exists. removing\n', ...

tmym);

rmdir(tmym, 's');

end

tmym = [which('mym') '/../../..'];

rmdir(tmym, 's');

guzman-raphael · 2020-09-14T19:11:07Z

+tests/TestSetupMYM.m

+            end
+
+            ms = setupMYM();
+            testCase.verifyTrue(strcmp(ms, 'master'));


Suggested change

testCase.verifyTrue(strcmp(ms, 'master'));

latest_mym = webread('https://api.github.com/repos/datajoint/mym/releases/latest');

testCase.verifyTrue(tests.lib.compareVersions([ms.major '.' ms.minor '.' ms.bugfix], latest_mym.tag_name(2:end)));

guzman-raphael · 2020-09-14T19:17:42Z

+tests/TestSetupMYM.m

+            testCase.verifyTrue(isdir(tmym));
+
+        end
+        function TestSetupMYM_testVersionInstallStale(testCase)


Not too clear on the objective of the below tests. Thinking you might only need 2 tests to cover most cases:

Install master, verify, install specific version w/o force, verify error, install specific version w/ force, verify

Install specific version, verify, install master w/o force, verify error, install master w/ force, verify

makes sense, good thinking, will adjust

guzman-raphael · 2020-09-14T19:31:21Z

setupMYM.m

@@ -0,0 +1,67 @@
+function setupMYM(version, force)


Thanks for the update. I will provide feedback on these points in my review where they associate within the code.

guzman-raphael · 2020-09-14T19:39:36Z

@ixcat One more note regarding tests on MySQL8, you will need to modify .travis.yml (here) to point to the new version-pinned image i.e. datajoint/mysql:8.0.18 to allow the tests to run properly until we address datajoint/mym#55.

ixcat · 2020-09-18T18:17:22Z

think version string handling is clear - remaining design questions:

setupMYM naming
why we are caching the function result for setupDJ to begin with, and if that reason also applies also to setupMYM

pending:

version string update per feedback
test link to main, test structure adjust, travis mysql version

ixcat · 2020-09-18T18:55:29Z

setupMYM naming: leaving as is, 'meh'+'sort conflicting criteria by something' as rationale
setupMYM caching to be removed

ixcat · 2020-10-09T15:47:26Z

so - since this effort will be deprecated, we need to clean up 'stage' - thoughts welcome to proceed.

guzman-raphael · 2020-10-09T16:14:47Z

Closing as discussed since #285 ready. Will reset stage.

ixcat added 4 commits June 19, 2020 10:49

+dj/Connection.m, setupDJ.m: move matlab, mym version checks into set…

74c54fe

…upDJ.m - ensures earlier detection of outdated components - part of fix for #228

setupDJ.m: use new setupMYM

a42397b

Merge pull request #246 from ixcat/issue-228

e4f6e1c

fix for #228 (Matlab / mYm improved version/compatibility support)

guzman-raphael requested changes Jul 31, 2020

View reviewed changes

ixcat added 9 commits July 31, 2020 18:40

{setupDJ,setupMYM}.m: minver 9.1/2016b, adjust print/error/warnings.

5c195b0

setupMYM: quiet no-reinstall warning for default case

e24c374

+= +tests/TestSetupMYM.m skeleton

602be47

setupMYM.m: document appropriate version strings

b888852

+= +tests/TestSetupMYM.m: prototype/WIP

3317587

Merge pull request #260 from ixcat/issue-228

de893bc

updates for #258 {setupDJ,setupMYM}.m: minver 9.1/2016b, adjust print/error/warnings.

setupMYM.m: return version string. fix nargin processing issue while …

727f49e

…here

setupMYM.m: += note on setupMYM version string vs mym('version')

3712243

Merge pull request #282 from ixcat/issue-228

d6ba4b7

updates for #258 {setupDJ,setupMYM}.m: version return, test case updates

ixcat added needs-discussion and removed needs-discussion labels Sep 12, 2020

guzman-raphael reviewed Sep 14, 2020

View reviewed changes

guzman-raphael closed this Oct 9, 2020

guzman-raphael mentioned this pull request Dec 8, 2020

Add Toolbox packaging #285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stage -> master for #246 (fix for #228 (Matlab / mYm improved version/compatibility support)) #258

stage -> master for #246 (fix for #228 (Matlab / mYm improved version/compatibility support)) #258

ixcat commented Jul 17, 2020

guzman-raphael left a comment •

edited

Loading

guzman-raphael Jul 31, 2020

ixcat Jul 31, 2020

guzman-raphael Jul 31, 2020

ixcat Jul 31, 2020

guzman-raphael Jul 31, 2020

ixcat Sep 12, 2020 •

edited

Loading

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

ixcat Sep 18, 2020

ixcat Sep 18, 2020

ixcat Sep 18, 2020

guzman-raphael Sep 14, 2020

ixcat Sep 18, 2020

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

ixcat Sep 18, 2020

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

guzman-raphael Sep 14, 2020

ixcat Sep 18, 2020

guzman-raphael Sep 14, 2020

guzman-raphael commented Sep 14, 2020

ixcat commented Sep 18, 2020 •

edited

Loading

ixcat commented Sep 18, 2020

ixcat commented Oct 9, 2020

guzman-raphael commented Oct 9, 2020

	function setupMYM(version, force)
	function mymVersion = setupMYM(version, force)

		@@ -0,0 +1,76 @@
		function mymVersion = setupMYM(version, force)


		persistent INSTALLED_VERSION;

		if ~isempty(INSTALLED_VERSION) && ~force

	testCase.verifyTrue(strcmp(ms, '2.7.2'));
	testCase.verifyTrue(strcmp([ms.major '.' ms.minor '.' ms.bugfix], '2.7.2'));

	testCase.verifyTrue(strcmp(ms, 'master'));
	latest_mym = webread('https://api.github.com/repos/datajoint/mym/releases/latest');
	testCase.verifyTrue(tests.lib.compareVersions([ms.major '.' ms.minor '.' ms.bugfix], latest_mym.tag_name(2:end)));

stage -> master for #246 (fix for #228 (Matlab / mYm improved version/compatibility support)) #258

stage -> master for #246 (fix for #228 (Matlab / mYm improved version/compatibility support)) #258

Conversation

ixcat commented Jul 17, 2020

guzman-raphael left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixcat Sep 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzman-raphael commented Sep 14, 2020

ixcat commented Sep 18, 2020 • edited Loading

ixcat commented Sep 18, 2020

ixcat commented Oct 9, 2020

guzman-raphael commented Oct 9, 2020

guzman-raphael left a comment •

edited

Loading

ixcat Sep 12, 2020 •

edited

Loading

ixcat commented Sep 18, 2020 •

edited

Loading