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

Wrap unzip function with bst_unzip to globally manage features #432

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

juangpc
Copy link
Collaborator

@juangpc juangpc commented Aug 17, 2021

Hi,

After finding some problems unzipping a zip file downloaded from github issue, I've found this solution. Not everyone is experiencing this problem (actually only me 😄 ) but, just in case.

Apparently Matlab is changing its unzip function while adapting to a java-less life. And this new version might not be as universally compatible as the old unzip version. While this new version becomes more stable, we can:

  • use this bst_unzip function which basically wraps the unzip call. I like this abstraction because I see no reason why any function using unzip should know or care about different versions of unzip, java dependencies, etc...
  • if a feature (i.e. ZIPV2) fails once, it is probably going to fail more than once. So it can globally be managed either through some settings parameter or some static variable inside the function ("persistent"). That's not implemented right now.
  • I see that there are 30 calls to unzip right now in the repo. Some with 1 input parameter (zipFilename) or 2 (zipFilename, outputFolder). bst_unzip is compatible with both formats.
  • Imho the complete switch here could be abstracted with a bst_unpackage file. I see no reason why plugin should know or care about zip/tar/ispc/system calls, etc... (this hasn't been done).

If you want to drop this. That's ok. Just trying to help.

@juangpc
Copy link
Collaborator Author

juangpc commented Aug 17, 2021

@tmedani you and some others can push modifications to this branch on my fork. Whatever suits you.

@tmedani
Copy link
Member

tmedani commented Aug 17, 2021

For me, it's fine Juan, since it handles more cases, including the current case.

@tmedani
Copy link
Member

tmedani commented Aug 18, 2021

We may need to ask @ftadel

@ftadel ftadel merged commit aa2c23f into brainstorm-tools:master Aug 18, 2021
@ftadel
Copy link
Member

ftadel commented Aug 18, 2021

Thanks.
But let's not invest more time in fixing all the calls to unzip(), since this looks like a very marginal bug that will ever exist only on one Matlab release on a handful of computers and for one specific zip file. If you see this happening in other places in the code, replace unzip() with bst_unzip() one occurrence at a time.

Imho the complete switch here could be abstracted with a bst_unpackage file. I see no reason why plugin should know or care about zip/tar/ispc/system calls, etc... (this hasn't been done).

Yes, we could. But so far bst_plugin is the only function that needs to unpackage files without knowing in advance what the compression format is, so it would only add an extra function without factorizing anything or reducing the amount of code.

@juangpc juangpc deleted the unzip_feature_gzipv2 branch August 18, 2021 15:18
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