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

BF: run_procedure: do not hardcode python executable (fixes gh-3623) #3624

Merged
merged 3 commits into from Aug 27, 2019

Conversation

@mih
Copy link
Member

commented Aug 26, 2019

Stay within the same Python version to avoid crash or undesired behavior
when PY2 is called from PY3 with missing or outdated PY2 versions of
datalad.

@@ -189,7 +190,7 @@ def _guess_exec(script_file):
'state': state}
elif script_file.endswith('.py'):
return {'type': u'python_script',
'template': u'python "{script}" "{ds}" {args}',
'template': u'%s "{script}" "{ds}" {args}' % sys.executable,

This comment has been minimized.

Copy link
@kyleam

kyleam Aug 26, 2019

Member

I suppose we should shlex_quote() sys.executable to guard against poorly named virtual environments with spaces.

This is outside the scope of this PR, but the quoting in these templates looks problematic. I think we should drop the quotes around the placeholders and pass each value through shlex_quote().

This comment has been minimized.

Copy link
@mih

mih Aug 26, 2019

Author Member

Good point, thx!

This comment has been minimized.

Copy link
@adswa

adswa Aug 27, 2019

Contributor

@kyleam I think I took care of this. I will open a new issue about

This is outside the scope of this PR, but the quoting in these templates looks problematic. I think we should drop the quotes around the placeholders and pass each value through shlex_quote().

and will try to PR a fix.

This comment has been minimized.

Copy link
@mih

mih Aug 27, 2019

Author Member

Windows is not happy. There is no shlex for windows it seems, and https://stackoverflow.com/questions/33560364/python-windows-parsing-command-lines-with-shlex suggests that doing it correctly is a project on its own. I'd say

from datalad.utils import on_windows
sys.executable if on_windows else shlex_quote(sys.executable)

This comment has been minimized.

Copy link
@kyleam

kyleam Aug 27, 2019

Member

Windows is not happy. There is no shlex for windows it seems

Eh, that means we need to deal with this in other spots that use shlex_quote.

Thanks for the updates, @adswa. This would not be a straightforward test for the test suite (and I'm not suggesting you add one), but I thought I should at least see that it works locally if I create a virtual environment with spaces. But I couldn't even get to that point because other things upstream (some outside of DataLad) were failing.

Anyway, outside of windows, shlex_quote at least gives us a better chance of working with weird names and doesn't hurt for non-weird names, so it seems worth keeping.

I'm going to rebase this branch onto 0.11.x because the sys.executable change is worth having there.

This comment has been minimized.

Copy link
@kyleam

kyleam Aug 27, 2019

Member

I'm going to rebase this branch onto 0.11.x because the sys.executable change is worth having there.

Done, with a minor spacing tweak to the last commit:

1:  97acf5f04 = 1:  dacb63257 BF: run_procedure: do not hardcode python executable (fixes gh-3623)
2:  94391e1f6 = 2:  b48290564 shlex quote sys.executable
3:  dc3628311 ! 3:  ece364ecc no shlexquote on windows
    @@ datalad/interface/run_procedure.py: def _guess_exec(script_file):
                      'state': state}
          else:
              return {'type': None, 'template': None, 'state': None}
    - 
    --
    - @build_doc
    - class RunProcedure(Interface):
    -     """Run prepared procedures (DataLad scripts) on a dataset
@codecov

This comment has been minimized.

Copy link

commented Aug 27, 2019

Codecov Report

Merging #3624 into 0.11.x will decrease coverage by 10.79%.
The diff coverage is 74.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #3624      +/-   ##
==========================================
- Coverage   81.23%   70.44%   -10.8%     
==========================================
  Files         256      273      +17     
  Lines       33819    35511    +1692     
==========================================
- Hits        27473    25015    -2458     
- Misses       6346    10496    +4150
Impacted Files Coverage Δ
datalad/tests/test_utils.py 96.92% <ø> (+0.4%) ⬆️
datalad/plugin/check_dates.py 40% <ø> (-28.58%) ⬇️
datalad/distribution/add.py 28.19% <ø> (-60.64%) ⬇️
datalad/support/tests/test_path.py 92.3% <ø> (+4.3%) ⬆️
datalad/tests/utils.py 89.29% <ø> (-0.29%) ⬇️
datalad/support/tests/test_repo_save.py 100% <ø> (ø)
datalad/tests/test_direct_mode.py 100% <ø> (+1.85%) ⬆️
datalad/support/tests/test_gitrepo.py 99.88% <ø> (ø) ⬆️
datalad/interface/__init__.py 100% <ø> (ø) ⬆️
datalad/support/tests/test_fileinfo.py 100% <ø> (ø)
... and 218 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4a3e25...ece364e. Read the comment docs.

@mih mih changed the title BF: run_procedure: do not hardcode python executable (fixes gh-3623) WIP: BF: run_procedure: do not hardcode python executable (fixes gh-3623) Aug 27, 2019

@mih mih changed the title WIP: BF: run_procedure: do not hardcode python executable (fixes gh-3623) BF: run_procedure: do not hardcode python executable (fixes gh-3623) Aug 27, 2019

@mih

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Thx @adswa and welcome!

mih and others added 3 commits Aug 26, 2019
BF: run_procedure: do not hardcode python executable (fixes gh-3623)
Stay within the same Python version to avoid crash or undesired behavior
when PY2 is called from PY3 with missing or outdated PY2 versions of
datalad.

@kyleam kyleam changed the base branch from master to 0.11.x Aug 27, 2019

@kyleam kyleam force-pushed the mih:bf-3623 branch from dc36283 to ece364e Aug 27, 2019

@mih

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Hey, @kyleam this fix is needed for a 0.12 deployment tomorrow. Would be good to be able to pull that from its final resting place in master.

@kyleam

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Hey, @kyleam this fix is needed for a 0.12 deployment tomorrow. Would be good to be able to pull that from its final resting place in master.

You'll be able to. I'll merge 0.11.x into master.

@mih

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Perfect, thx!

kyleam added a commit that referenced this pull request Aug 27, 2019

@kyleam kyleam merged commit ece364e into datalad:0.11.x Aug 27, 2019

3 checks passed

WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kyleam added a commit to adswa/datalad that referenced this pull request Aug 28, 2019
TST: run_procedure: Mark test_quoting as known windows failure
This test fails on AppVeyor [0].  run_procedure() uses a no-op
shlex_quote() on Windows [1], so this is expected.

[0]: https://ci.appveyor.com/project/mih/datalad/builds/27022010/job/jq3lr977ht5vrmpj#L1109
[1]: datalad#3624 (comment)
kyleam added a commit to adswa/datalad that referenced this pull request Aug 28, 2019
TST: run_procedure: Mark test_spaces() as known Windows failure
run_procedure() can't use shlex_quote() on Windows [0], so an argument
with spaces will not be handled correctly [1].  We should probably
teach the windows branch of maybe_shlex_quote() to handle simple cases
like a name with spaces.

[0]: datalad#3624 (comment)
[1]: https://ci.appveyor.com/project/mih/datalad/builds/27022362/job/xoum5f43c135bfgd#L1125
yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7': (87 commits)
  DOC: Added an entry to changelogn on merged 3631
  ENH: finalizing changelog for 0.11.7
  TST: Update tests for a git-annex without direct mode
  TST: utils: Add decorator that skips when direct mode is unsupported
  ENH: annexrepo: Refuse to initialize in direct mode if unsupported
  ENH: annexrepo: Add check_direct_mode_support method
  BF+TST: Avoid leaking patched git-annex version
  TST+RF: test_annexrepo: Split up a test
  CHANGELOG.md: Second batch for 0.11.7
  TST: run_procedure: Mark test_spaces() as known Windows failure
  TST: run_procedure: Mark test_quoting as known windows failure
  TST: run_procedure: Test more arguments that need quoting
  BF(py2): run_procedure: Avoid encoding error in log message
  TST: add run_procedure test with spaces in file name
  TST/RF: non-hardcoded Python executable
  RF: newline at end of file
  RF: helper instead of conditional
  RF: remove superfluous imports
  BF/TST: remove quoting
  ENH: replace conditionals with helper function
  ...
yarikoptic added a commit that referenced this pull request Sep 6, 2019
Merge tag '0.11.7' into debian
0.11.7 (Sep 02, 2019) -- python2-we-still-love-you-but-...

Primarily bugfixes with some optimizations and refactorings.

 Fixes

- [addurls][]
  - now provides better handling when the URL file isn't in the
    expected format.  ([#3579][])
  - always considered a relative file for the URL file argument as
    relative to the current working directory, which goes against the
    convention used by other commands of taking relative paths as
    relative to the dataset argument.  ([#3582][])

- [run-procedure][]
  - hard coded "python" when formatting the command for non-executable
    procedures ending with ".py".  `sys.executable` is now used.
    ([#3624][])
  - failed if arguments needed more complicated quoting than simply
    surrounding the value with double quotes.  This has been resolved
    for systems that support `shlex.quote`, but note that on Windows
    values are left unquoted. ([#3626][])

- [siblings][] now displays an informative error message if a local
  path is given to `--url` but `--name` isn't specified.  ([#3555][])

- [sshrun][], the command DataLad uses for `GIT_SSH_COMMAND`, didn't
  support all the parameters that Git expects it to.  ([#3616][])

- Fixed a number of Unicode py2-compatibility issues. ([#3597][])

- [download-url][] now will create leading directories of the output path
  if they do not exist ([#3646][])

 Enhancements and new features

- The [annotate-paths][] helper now caches subdatasets it has seen to
  avoid unnecessary calls.  ([#3570][])

- A repeated configuration query has been dropped from the handling of
  `--proc-pre` and `--proc-post`.  ([#3576][])

- Calls to `git annex find` now use `--in=.` instead of the alias
  `--in=here` to take advantage of an optimization that git-annex (as
  of the current release, 7.20190730) applies only to the
  former. ([#3574][])

- [addurls][] now suggests close matches when the URL or file format
  contains an unknown field.  ([#3594][])

- Shared logic used in the setup.py files of Datalad and its
  extensions has been moved to modules in the _datalad_build_support/
  directory.  ([#3600][])

- Get ready for upcoming git-annex dropping support for direct mode
  ([#3631][])

* tag '0.11.7':
  Changelog entry for download-url paths handling
  ENH: downloaders: Ensure directories for target exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.