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

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

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions +dj/Connection.m
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@
% initQuery is the SQL query to be executed at the start
% of each new session.
setupDJ(true);
try
mymVersion = mym('version');
assert(mymVersion.major > 2 || mymVersion.major==2 && mymVersion.minor>=6)
catch
error 'Outdated version of mYm. Please upgrade to version 2.6 or later'
end
if verLessThan('matlab', '8.6')
error 'MATLAB version 8.6 (R2015b) or greater is required'
end
self.host = host;
self.user = username;
self.password = password;
Expand Down
117 changes: 117 additions & 0 deletions +tests/TestSetupMYM.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@

classdef TestSetupMYM < tests.Prep
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

% TestSetupMyM tests setupMYM

methods (Test)
function TestSetupMYM_testDefaultInstall(testCase)

[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
Comment on lines +8 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');


ms = setupMYM();
testCase.verifyTrue(strcmp(ms, 'master'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)));

testCase.verifyTrue(isdir(tmym));

end
function TestSetupMYM_testVersionInstallFresh(testCase)
[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if isdir(tmym)
fprintf('testVersionInstallFresh: removing mymdir %s\n', ...
tmym);
rmdir(tmym, 's');
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

testCase.verifyTrue(isdir(tmym));

end
function TestSetupMYM_testVersionInstallStale(testCase)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, good thinking, will adjust

[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if ~isdir(tmym) % XXX: valid? how handle otherwise?
fprintf('testVersionInstallStale: spoofing mymdir %s\n', ...
tmym);
mkdir(tmym);
testCase.verifyTrue(isdir(tmym));
end

% TODO: how manage version string?
ms = setupMYM('2.7.2'); % TODO: how properly verify?
% also: .. persistent & test state ...

end
function TestSetupMYM_testVersionInstallStaleForce(testCase)
[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if ~isdir(tmym) % XXX: valid? how handle otherwise?
fprintf('testVersionInstallStaleForce: spoofing mymdir %s\n', ...
tmym);
mkdir(tmym);
testCase.verifyTrue(isdir(tmym));
end

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

end
function TestSetupMYM_testMasterInstallFresh(testCase)
[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if isdir(tmym)
fprintf('testMasterInstallFresh: removing mymdir %s\n', ...
tmym);
rmdir(tmym, 's');
end

ms = setupMYM('master');
testCase.verifyTrue(strcmp(ms, 'master'));
testCase.verifyTrue(isdir(tmym));

end
function TestSetupMYM_testMasterInstallStale(testCase)
[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if ~isdir(tmym) % XXX: valid? how handle otherwise?
fprintf('testMasterInstallStale: spoofing mymdir %s\n', ...
tmym);
mkdir(tmym);
testCase.verifyTrue(isdir(tmym));
end

ms = setupMYM('master'); % TODO: how verify fail?
% also: .. persistent & test state ...
end
function TestSetupMYM_testMasterInstallStaleForce(testCase)
[dir, nam, ext] = fileparts(mfilename('fullpath'));
tmym = strcat(dir, '/../', 'mym');

if ~isdir(tmym) % XXX: valid? how handle otherwise?
fprintf('testMasterInstallStaleForce: spoofing mymdir %s\n', ...
tmym);
mkdir(tmym);
testCase.verifyTrue(isdir(tmym));
end

ms = setupMYM('master');
testCase.verifyTrue(strcmp(ms, 'master'));
testCase.verifyTrue(isdir(tmym));

end
end
end
41 changes: 17 additions & 24 deletions setupDJ.m
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,31 @@ function setupDJ(skipPathAddition, force)
return
end

if verLessThan('matlab', '9.1')
error('DataJoint:System:UnsupportedMatlabVersion', ...
'MATLAB version 9.1 (R2016b) or greater is required');
end

base = fileparts(mfilename('fullpath'));

if nargin < 1
skipPathAddition = false;
end

if ~skipPathAddition
fprintf('Adding DataJoint to the path...\n')
addpath(base)
fprintf('Adding DataJoint to the path...\n');
addpath(base);
end

mymdir = fullfile(base, 'mym');
% if mym directory missing, download and install
if ~isdir(mymdir)
fprintf('mym missing. Downloading...\n')
target = fullfile(base, 'mym.zip');
mymURL = 'https://github.com/datajoint/mym/archive/master.zip';
target = websave(target, mymURL);
if isunix && ~ismac
% on Linux Matlab unzip doesn't work properly so use system unzip
system(sprintf('unzip -o %s -d %s', target, base))
else
unzip(target, base)
end
% rename extracted mym-master directory to mym
movefile(fullfile(base, 'mym-master'), mymdir)
delete(target)
setupMYM('master', force);

try
mymVersion = mym('version');
assert(mymVersion.major > 2 || mymVersion.major==2 && mymVersion.minor>=6);
catch
error('DataJoint:System:UnsupportedMyMVersion', ...
'Outdated version of mYm. Please upgrade to version 2.6 or later');
end

% run mymSetup.m
fprintf('Setting up mym...\n')
run(fullfile(mymdir, 'mymSetup.m'))


INVOKED = 1;
end
76 changes: 76 additions & 0 deletions setupMYM.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
function mymVersion = setupMYM(version, force)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


default_version = 'master'; % else git tag string e.g. '2.7.2'

if nargin < 1
version = default_version;
end
if nargin < 2
force = false;
end

persistent INSTALLED_VERSION;

if ~isempty(INSTALLED_VERSION) && ~force
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

% 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
end

base = fileparts(mfilename('fullpath'));

mymdir = fullfile(base, 'mym');

if isdir(mymdir)
if force
fprintf('force install.. removing %s\n', mymdir);
rmdir(mymdir, 's');
elseif(~strcmp(version, default_version))
warning('DataJoint:System:setupMyMwarning', ...
['Warning: mym directory exists. not re-installing.\n', ...
' to override, pass force=true\n']);
end
end

if ~isdir(mymdir) %% mym directory missing, download and install
ixcat marked this conversation as resolved.
Show resolved Hide resolved
fprintf('Installing %s...\n', version);
target = fullfile(base, 'mym.zip');

mymURL = 'https://github.com/datajoint/mym/archive/';

if strcmp(version, 'master')
mymURL = strcat(mymURL, version, '.zip');
else
mymURL = strcat(mymURL, 'v', version, '.zip');
end

fprintf('downloading %s to %s\n', mymURL, target);
target = websave(target, mymURL);

extdir = fullfile(base, sprintf('mym-%s', version));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ 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.

fprintf('extracting %s into %s\n', target, extdir);

if isunix && ~ismac
% on Linux Matlab unzip doesn't work properly so use system unzip
system(sprintf('unzip -o %s -d %s', target, base));
else
unzip(target, base);
end

% rename extracted mym-master directory to mym
fprintf('renaming %s to %s\n', extdir, mymdir);
movefile(extdir, mymdir);

delete(target);
end

% run mymSetup.m
fprintf('Setting up mym...\n');
run(fullfile(mymdir, 'mymSetup.m'));

mymVersion = version;
INSTALLED_VERSION = mymVersion;

end