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

Use shlex's posix=False for Windows compatibility #84

Closed
wants to merge 1 commit into from
Closed

Use shlex's posix=False for Windows compatibility #84

wants to merge 1 commit into from

Conversation

kousu
Copy link
Contributor

@kousu kousu commented May 26, 2020

Fixes #83.

I'm a little bit worried this will cause problems on Unix. I tried it quickly and it didn't break my examples.

Maybe utils.run_shell_command() should be dropped in favour of calling subprocess.Popen() directly. It's a lot safer that way; less worries about quoting issues turning into exploits.

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #84 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage   70.52%   70.52%           
=======================================
  Files           9        9           
  Lines         587      587           
  Branches       75       75           
=======================================
  Hits          414      414           
  Misses        149      149           
  Partials       24       24           
Flag Coverage Δ
#pytest 70.52% <0.00%> (ø)
Impacted Files Coverage Δ
dcm2bids/utils.py 68.96% <0.00%> (ø)

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 24b2831...2425654. Read the comment docs.

@kousu
Copy link
Contributor Author

kousu commented May 26, 2020

With this patch, my reproduction in #83 now runs:

C:\Users\user\src\Dcm2bids>dcm2bids -d data_testing\data_testing\dicom_unsorted -o output -p "" -c example\config.json
INFO:dcm2bids.dcm2bids:--- dcm2bids start ---
INFO:dcm2bids.dcm2bids:OS:version: Windows-10-10.0.18362-SP0
INFO:dcm2bids.dcm2bids:python:version: 3.8.3 (tags/v3.8.3:6f8c832, May 13 2020, 22:20:19) [MSC v.1925 32 bit (Intel)]
INFO:dcm2bids.dcm2bids:dcm2bids:version: 2.1.4
INFO:dcm2bids.dcm2bids:dcm2niix:version: v1.0.20200331
INFO:dcm2bids.dcm2bids:participant: sub-
INFO:dcm2bids.dcm2bids:session:
INFO:dcm2bids.dcm2bids:config: C:\Users\user\src\Dcm2bids\example\config.json
INFO:dcm2bids.dcm2bids:BIDS directory: C:\Users\user\src\Dcm2bids\output
INFO:dcm2bids.utils:Running dcm2niix -b y -ba y -z y -f '%3s_%f_%p_%t' -o output\tmp_dcm2bids\sub- data_testing\data_testing\dicom_unsorted
INFO:dcm2bids.dcm2niix:Check log file for dcm2niix output
INFO:dcm2bids.sidecar:Sidecars pairing:
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e1
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e1_ph
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e2
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e2_ph
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e3
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e3_ph
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e4
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e4_ph
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e5
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e5_ph
INFO:dcm2bids.sidecar:No Pairing  <-  '006_dicom_unsorted_a_gre_DYNshim_20191101153345'_e6
INFO:dcm2bids.sidecar:No Pairing  <-  '007_dicom_unsorted_a_gre_DYNshim_20191101153345'_e6_ph
INFO:dcm2bids.dcm2bids:moving acquisitions into BIDS folder

@kousu
Copy link
Contributor Author

kousu commented May 26, 2020

I'm not sure what codecov is complaining about. That the patch isn't tested? But that function wasn't tested before either.

@kousu
Copy link
Contributor Author

kousu commented Nov 17, 2020

My solution is bad! https://stackoverflow.com/a/35900070

shlex.split(cmd, posix=0) retains backslashes in Windows paths, but it doesn't understand quoting & escaping right. Its not very clear what the posix=0 mode of shlex is good for at all - but 99% it certainly seduces Windows/cross-platform programmers ...

https://bugs.python.org/issue1724822 - last updated, 2011 :*(

@kousu
Copy link
Contributor Author

kousu commented Nov 18, 2020

Bad solution. I'm doing what I suggested in #83 instead.

@kousu kousu closed this Nov 18, 2020
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.

Windows file paths
1 participant