dsa-ou / m269-installer

Software installation script and instructions for M269
https://dsa-ou.github.io/m269-installer/
BSD 3-Clause "New" or "Revised" License
1 stars 2 forks source link

Windows: comma (and maybe other special chars) in M269 folder file path causes `allowed` alias to fail #34

Closed densnow closed 1 year ago

densnow commented 1 year ago

A comma, or possibly other special characters in the M269 file path cause the allowed alias to fail. The other aliases are unaffected.

I believe the following will fix the issue:

$CONFIG_FILE = $Profile.CurrentUserCurrentHost
- $ESC = "```""
$ALIASES = @"
function m269-23j {
    cd '$FOLDER'
    & '$VENV\Scripts\Activate.ps1'
}
function nb {
    Start-process -NoNewWindow jupyter -ArgumentList "notebook"
}
function allowed {
    param(
        [string]`$FilePath
    )

-    python $ESC$FOLDER\allowed.py$ESC -c $ESC$FOLDER\m269.json$ESC `$FilePath
+    python '$FOLDER\allowed.py' -c '$FOLDER\m269.json' `$FilePath
}
"@
mwermelinger commented 1 year ago

What was the reason you used triple-ticks in the first place? We don't want this fix to (re-)create other issues :-)

mwermelinger commented 1 year ago

In particular, does it still work with spaces in the path?

densnow commented 1 year ago

It wasn't me that added the triple-ticks, so I have no idea😀, although I did approve the PR, but that was only to test for spaces and not commas!

I do believe this works with spaces(have tested), but I will test more times to make sure.

densnow commented 1 year ago

I think this issue and fix highlights the need for some kind of testing script(s) so we can make sure some a small fix like this doesn't break the whole thing. Although the cost to create the scripts in the first place is high (they would probably be longer than the install scripts...) and I am sure you are busy enough already. 😀

Maybe something for the future?

mwermelinger commented 1 year ago

@ICTSoEasy Back some time ago you added triple-backticks to make the PowerShell allowed alias work for path names with spaces. Do you remember why you didn't just use simple quotes? It seems the simple quote works for paths with spaces and commas but the triple-backtick only for spaces. Is it because some paths can have quotes themselves the expanded in the script?

densnow commented 1 year ago

Ok so according to MS docs (https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#file-and-directory-names) double quotes ( " ) are not allowed in file paths, so they would be the better choice here over single quotes ( ' )

AFAIK triple-ticks do nothing special in powershell, single ticks will escape a character like they are doing in front of the $ signs, but there is nothing said anywhere about triple-ticks

Double quotes as path delimiters works with both spaces and commas.

ICTSoEasy commented 1 year ago

Yes I think that was the case – there was some reason that simple quotes just wouldn’t work. Similarly with putting them in variables rather than directly in the path, the expansion was causing problems!

mwermelinger commented 1 year ago

@densnow Could you change to double-quotes, test with commas, spaces and single-quotes in the path, and then do a PR? Thanks.

densnow commented 1 year ago

@mwermelinger Yes, I will have a look at this over the weekend.

densnow commented 1 year ago

Just going to add this link here for future reference regarding quotes etc in powershell https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-7.3

The double quoted "here-string" (multiline), strings that start and end with @ do the following :