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

A new installation workflow for PTB #268

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bpancras
Copy link
Contributor

This contribution implements a new installation workflow for Psychtoolbox centered around GitHub's Rest API. This is a prototype implementation targeted towards replacing the now deprecated SVN based installation workflow. The implementation has the following interface:

installPTB
This installs the latest release of Psychtoolbox.

installPTB('ver', '3.0.19.6')
This will install version 3.0.19.6.

The installPTB script can be used until version 3.0.16.8, the tagging convention was different before this release.

"THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY."

@vijayiyer05
Copy link

Hi @bpancras, looks like @kleinerm has force-pushed this PR, which I didn't catch earlier.
Is there additional information/action you may need/want from the PTB side? I'm not as familiar with force-pushes.

@kleinerm
Copy link
Owner

Hi @bpancras and @vijayiyer05

sorry for the late reply. I was forced by my boss, without meaningful heads up, to use up all saved up remaining vacation days of the last years 2022-2023, of which there were many, or lose all of them without any compensation. This lead to a mostly complete stand-still of most not-super-critical work on PTB since early March, and now to being behind in schedule by over 1.5 months on important projects. Hence this delay, and likely future delays in the next months, sorry.

Anyhow, I just had a quick skim over this, and while I thank you for trying, this contribution is not suitable in its current form.

  1. As stressed in earlier e-mails to you from many months ago, sometimes last year, and in the online meeting we once had, any such installer must also reliably work with GNU/Octave, version 5.2 or later at this point, not only with the most recent versions of Matlab. Otherwise it is a big step back for us and just increases confusion of our users and software-maintenance and user-support overhead. Even the latest Octave does not support the websave() function yet, and webread() is not available in Octave 5.2 - shipping with Ubuntu Linux 20.04 and similar, so even webread() can not be used until we drop support for Octave 5.x completely. I don't know about the status/capabilities/compatibility of the used inputParser() function either. Anything that uses OOP functionality may be shaky in some cases. There may be other troublemakers. Use of a closed-source function like getAddOnsFolder.p is also not acceptable and certainly permanently incompatible with Octave.

  2. I don't see sufficient error handling and guidance and help for users. Especially installer/setup functionality must be extra careful.

  3. As we don't want new installer functions in the first place, whatever is done would have to be implemented inside the existing DownloadPsychtoolbox.m and - if applicable - UpdatePsychtoolbox.m functions, which already provide help texts, user guidance, relatively careful error handling and all the post-install routines and mechanisms. This is what users are used to from the past. Ideally we'd have a replacement for the old Subversion based install/update methods, just directly based on Git, but failing that, something that is compatible with Octave. E.g., the older urlread() and urlwrite() functions seem to be supported by both Octave and Matlab, although compatibility would have to be tested and taken into account.

If you look at the svndownload() subfunction inside DownloadPsychtoolbox.m and its use and implementation, I think that would be roughly what would need to be reimplemented, ideally via Git access, or via urlread/urlwrite variants of your approach. The function as a whole will need some cleanup, but the meat of it is a svndownload subfunction replacement that is fully compatible with Octave 5.2 and later and Matlab R2014b and later.

@kleinerm
Copy link
Owner

A few more comments after playing with bits of code under Octave 8.4. I assume stuff works hopefully the same with Octave 6.1 and later, which would be required.

webread() is supported in Octave 6.1 and later, which would be good enough for our purposes. The way to get releaseJSON on Octave, as used in your code, would be via releaseJSON = jsondecode(webread(...)); instead of releaseJSON = webread(...);, cfe. IsOctave() function for detecting Octave vs. Matlab to choose the right branch in an if-else-end for this. However, jsondecode only exists on Octave 7.1, not on Octave 6.1 which we still need to support for Ubuntu 22.04, so that's a problem.

urlwrite() works as replacement for webwrite() in Octave.

You would not want to store the zip file in the Matlab Add Ons folder in the first place, as no such thing exists on Octave or in older Matlab's, and .mltbx files installed via the Add Ons explorer would be the way to go anyway in that case. So that whole logic can go and one uses the targetfolder specified by the user in DownloadPsychtoolbox.m etc.

Finally, that's the wrong zip file to use, as it contains also all the C-Source, build scripts, Python code etc. You'd want the 3.0.x.y.zip file, e.g., from the url:

downloadurl = releaseJSON{i}.assets(2).browser_download_url;

Btw. for some unfortunate reason, the mltbx files also seem to contain the source folders etc., despite them not being specified in the toolbox project file. Only the Psychtoolbox/ subfolder should be part of those.

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

Successfully merging this pull request may close these issues.

3 participants