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

plots: quote and resolve paths properly #5670

Merged
merged 1 commit into from Mar 24, 2021

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Mar 22, 2021

This commit fixes 3 things:

  1. The path printed on the terminal is properly is quoted. This was
    problematic when the output is piped to xdg-open if the path contained
    spaces or quotes or special characters.
$ dvc plots show -o 'plots with spaces.html' | xargs xdg-open
xdg-open: unexpected argument 'with'
Try 'xdg-open --help' for more information.
  1. The path is resolved. This is not an issue at all but makes the
    output look nicer.
$ dvc plots show -o '../plots.html' --open
file:///home/saugat/repos/iterative/example-get-started/../plots.html
  1. The path with which the browser is opened with uses relative path.
    This is done to solve issue in WSL, that the absolute path is not
    accessible outside the WSL for host programs as the linux filesystem
    is available on /wsl$ path.
$ dvc plots show --open
file:///home/saugat/example-get-started/plots.html
Start : This command cannot be run due to the error: The system cannot find the file specified.
At line:1 char:1
+ Start "/home/saugat/example-get-started/plots.html"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [Start-Process], InvalidOperationException
    + FullyQualifiedErrorId : InvalidOperationException,Microsoft.PowerShell.Commands.StartProcessCommand
  1. On Windows, the file-uri was being constructed using platform-dependent os.path.join. This has been changed to use as_uri() from pathlib.Path.

With the fix to use relative path as argument to webbrowser.open, I expect WSL to be able to work from outside (tested) as well as within inside if needed (didnot test at all). This does work on Linux.

See #5584 (comment)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@skshetry skshetry added the bugfix fixes bug label Mar 22, 2021
@skshetry skshetry requested a review from pared March 22, 2021 16:40
@skshetry skshetry self-assigned this Mar 22, 2021
@skshetry skshetry added this to In progress in DVC 9 - 23 March 2021 via automation Mar 22, 2021
@skshetry skshetry force-pushed the fix-plots-path branch 2 times, most recently from 3de011b to 2b689d4 Compare March 23, 2021 11:11
@skshetry
Copy link
Member Author

Need to do a QA on Windows for the file-uri changes. Previously, the file-uri where being constructed through platform-specific paths, which means there were \\ in them. Needs to be checked if those were valid before.

@efiop efiop added this to In progress in DVC 23 March - 06 April 2021 via automation Mar 23, 2021
@efiop efiop moved this from In progress to Done in DVC 9 - 23 March 2021 Mar 23, 2021
@efiop efiop moved this from In progress to Review in progress in DVC 23 March - 06 April 2021 Mar 23, 2021
This commit fixes 3 things:
1. The path printed on the terminal is properly is quoted.  This was
problematic when the output is piped to `xdg-open` if the path contained
spaces or quotes or special characters.

2. The path is resolved. This is not an issue at all, but makes the
output nicer.

3. The path with which the browser is opened with uses relative path.
This is done to solve issue in WSL, that the absolute path is not
accessible outside the WSL for host programs as the linux filesystem
is available on `/wsl$` path.
@skshetry
Copy link
Member Author

skshetry commented Mar 24, 2021

Was not able to test out on windows, I was curious if it worked before there, but probably it didn't. 🤞🏼

@skshetry skshetry merged commit 76ece8c into iterative:master Mar 24, 2021
DVC 23 March - 06 April 2021 automation moved this from Review in progress to Done Mar 24, 2021
@skshetry skshetry deleted the fix-plots-path branch March 24, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes bug
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants