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

BUG: Fix passing module paths containing single quote #1023

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jamesobutler

jamesobutler commented Sep 19, 2018

Fixes https://issues.slicer.org/view.php?id=4592

This is a proposed fix and will need help merging into master by a main Slicer developer. Hopefully this can be included before the upcoming Slicer release.

@lassoan or @jcfr could you take a look at this? Thanks!

@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 19, 2018

Nice catch, thanks for proposing a fix.

It would be better to address the root cause of the issue - special characters in input string - rather than just slightly tuning quotation style.

Could you add a toPythonStringLiteral helper function (maybe a method or static function in qSlicerCorePythonManager) that would convert a string to a safe Python string literal? For now, you could encode only the ' character by replacing it by \'. In the future, we could add more escape sequence replacements as needed.

We can leave the CMakeLists.txt in that test as is, just add a comment there that explains why we had to use \" instead of '.

@jamesobutler

This comment has been minimized.

jamesobutler commented Sep 20, 2018

@lassoan I've pushed a second commit so you can see the changes I've made. This should include the updates as you suggested.

@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 20, 2018

Thank you, it is almost perfect. The only thing I would change is to return with a complete string literal (including quotation marks). So that you can use it like this:

this->executeString(QString("import sys; sys.path.append(%1); del sys").arg(this->toPythonStringLiteral(path))); 

and

QString("import imp;imp.load_source(%2, %1);del imp;") 
      .arg(pythonManager->toPythonStringLiteral(fileName))
      .arg(pythonManager->toPythonStringLiteral(moduleName)).toLatin1()

and

      QString("with open(%1, 'rb') as f:import imp;imp.load_module(%2, f, %1, ('.pyc', 'rb', 2));del imp") 
      .arg(pythonManager->toPythonStringLiteral(fileName))
      .arg(pythonManager->toPythonStringLiteral(moduleName)).toLatin1(),
@@ -50,6 +50,9 @@ class Q_SLICER_BASE_QTCORE_EXPORT qSlicerCorePythonManager : public ctkAbstractP
/// Append \a paths to \a sys.path
void appendPythonPaths(const QStringList& paths);
// /// Convert a string to a safe python string literal

This comment has been minimized.

@lassoan

lassoan Sep 20, 2018

Contributor

Keep only 3 slashes instead of 5.
Describe which quote is used for the literal (" or '). Probably ' would be better (later quote character can be made configurable).

@jamesobutler jamesobutler force-pushed the jamesobutler:4592-passing-paths branch from 84e35da to fc3fcce Sep 20, 2018

@jamesobutler

This comment has been minimized.

jamesobutler commented Sep 20, 2018

I've pushed an update, but for some reason scripted modules in the additional module paths aren't getting loaded. I keep getting this error from within qSlicerScriptedUtils though the output in that message looks fine. Not sure what I'm doing wrong here.

@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 21, 2018

Thank you! I've made some small changes and added tests. I'll double check that all tests pass and push the changes here so that you can review.

ENH: Improved qSlicerCorePythonManager::toPythonStringLiteral
Added more escape sequences, more information to method documentation, Python wrapping, and test.
@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 21, 2018

@jamesobutler please have a look at and test the latest commit. If it is OK for you then let me know and I merge the pull request.

@jamesobutler

This comment has been minimized.

jamesobutler commented Sep 21, 2018

Tested on the original computer that had the problem of a single quote in the user name and it appears additional module paths load correctly. Thanks for improving this and adding the test cases!

With your updates I was trying to simulate the single quote in the path issue on my system by after installing an extension moving the contents of %AppData%\NA-MIC\Extensions-27422 to "C:\D\James's" directory and updating the "Additional modules paths" in settings, but for some reason it can't load the qt-loadable-modules in those paths. Maybe just an issue with how I copied the files. See attached log with paths for SlicerOpenIGTLink extension.
Slicer_27422_20180921_142049.log

@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 21, 2018

Thanks for testing. SlicerOpenIGTLink may rely on DLLs in the bin folder (igtlio*.dll, OpenIGTLink.dll, igtlutil.dll), so maybe paths for these additional binaries are not set up correctly. Could you try if it works for non-superbuild extensions and Python-only extensions?

@jamesobutler

This comment has been minimized.

jamesobutler commented Sep 21, 2018

So I installed SegmentEditorExtraEffects (and its dependency MarkupsToModel) and moved them to the new path with the single quote and updated the additional module paths.

The Python-only SegmentEditorExtraEffects appears to have imported all correctly. MarkupsToModel a qt-loadable-module with qSlicerMarkupsToModel*.dll and vtkSlicerMarkupsToModel*.dll also appears to have imported correctly. So you might be right that it might be a problem specific to the DLLs in SlicerOpenIGTLink.

@lassoan

This comment has been minimized.

Contributor

lassoan commented Sep 22, 2018

I've tested extension install and run with the user name O'Test and it all worked well.

Integrated in e40af17

I've set you as author in the commit in git, but it seems that git-svn does not propagate author information information from git to svn.

Thanks for your contribution!

@lassoan lassoan closed this Sep 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment