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

Quote file names in ConvertUI #2793

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Quote file names in ConvertUI #2793

merged 4 commits into from
Mar 11, 2024

Conversation

MariannaGerina
Copy link
Contributor

Description

Please include a summary of the change and which issue is fixed. List any dependencies that are required for this change.
Fixed issue: convertUI does not create a outfiles when a space is present in a folder of the path. The issue was fixed properly quoting the files name in the convert UI.
Fixes #2791

How Has This Been Tested?

After this change, run.py can start the application

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@llimeht
Copy link
Contributor

llimeht commented Jan 24, 2024

Good catch, and I suspect there are others like this elsewhere in the code.

Trying to robustly quote filenames will always be problematic (what happens if my directory name has a " in it?) and os.system passes the command through a subshell which can do odd things too (what if the directory name has a $ or ` in it?). os.system also look after the output (just gets dumped to the terminal, probably not an issue for these uses). The current code also doesn't check that the operation succeeded, which it really should.

Rather than trying to quote filenames to put them in a string and give them to os.system so that the shell can then try to split the string back up and then execute the command, how about putting the command into a list and giving it directly to subprocess.run. This has the benefit that the command doesn't get interpreted by a shell (with the default shell=False) so no quoting is needed. Additionally, output is easily captured, and it can raise an exception on failed operations.

https://docs.python.org/3/library/subprocess.html#subprocess.run

@butlerpd
Copy link
Member

I think this should be against 6.0.0 not main. Particularly given the comments by @llimeht who points out that this is a tiny band-aid for a much larger problem than continuously bites us. However his suggestion seems like it will require skills beyond mine or the author's and the question is whether anybody else will be able to address this "properly" in the near future? If not I would recommend we merge this into 6.0.0 and create an issue with @llimeht suggestions given it fixes a problem we had at the contributor camp?

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Feb 13, 2024
@butlerpd
Copy link
Member

@lucas-wilkins suggests fixing the way @llimeht suggests but only for this issue before merging "should not be hard to do" - but says it is better than no change.

@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Feb 13, 2024
@butlerpd
Copy link
Member

@lucas-wilkins suggests this will be "easy" ... looking this up and reading a bit about it do you mean we should replace

run_line = f"pyside6-rcc {in_file} -o {out_file}"
os.system(run_line)

with

run_line = ["fpyside6-rcc", in_file, "-o" ,out_file]
subprocess.run(run_line)

??
It doesn't seem like any of the kwargs are necessary here? all defaults should be good?
This seems a bit scary to a white belt like me?

@lucas-wilkins
Copy link
Contributor

@butlerpd Yeah, looks right to me.

@butlerpd
Copy link
Member

OK thanks ... I'll give it a go then

@butlerpd butlerpd changed the base branch from main to release_6.0.0 March 1, 2024 17:04
@butlerpd butlerpd merged commit f925d6a into release_6.0.0 Mar 11, 2024
36 checks passed
@butlerpd butlerpd deleted the 2791-convertUI-fix branch March 11, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants