msvc-wine icon indicating copy to clipboard operation
msvc-wine copied to clipboard

add double-quotes to shell scripts

Open panzi opened this issue 3 years ago • 8 comments

I think this should make it possible to install it to a path that contains spaces. (Haven't tested it, had no time for that today.)

panzi avatar Jan 26 '22 02:01 panzi

I think this should make it possible to install it to a path that contains spaces. (Haven't tested it, had no time for that today.)

Please do test, I don't want to merge this blindly if it wouldn't actually fix the thing it sets out to fix (even if more quoting generally is good).

mstorsjo avatar Feb 07 '22 22:02 mstorsjo

Ok, tested it now. There was indeed some place where the spaces weren't properly handled yet. I've added replacevars.py to do correct replacements in msvcenv.sh. This could probably also be done with perl or awk, but I know how to do it in Python. I've squashed the fixes to my earlier commit so its still only one commit so I don't pollute your history.

panzi avatar Feb 08 '22 02:02 panzi

I've added replacevars.py to do correct replacements in msvcenv.sh. This could probably also be done with perl or awk, but I know how to do it in Python.

Hmm, I'm not a huge fan of adding an extra script just for doing those replacements. But in any case, I was planning another refactoring, to not hardcode BASE and BASE_UNIX as absolute paths, but calculate them at runtime, to allow moving around the installed toolchain. With that change in place (I just pushed it as f983e0be893f588a3360e7384dde930ea23b6e2b), there shouldn't be any need to change how the replacements are done, and the preexisting sed based replacement should work just fine.

I've squashed the fixes to my earlier commit so its still only one commit so I don't pollute your history.

Thanks, that's appreciated, that's my preferred form of working too!

mstorsjo avatar Feb 13 '22 21:02 mstorsjo

Ah, I see there are new (conflicting) changes. I'll rebase my branch to that.

panzi avatar Feb 13 '22 22:02 panzi

I've added replacevars.py to do correct replacements in msvcenv.sh. This could probably also be done with perl or awk, but I know how to do it in Python.

Hmm, I'm not a huge fan of adding an extra script just for doing those replacements. [...] With that change in place (I just pushed it as f983e0b), there shouldn't be any need to change how the replacements are done, and the preexisting sed based replacement should work just fine.

I'll reiterate this point just to make it clear. I don't want to add a separate script to do the replacements, when the current in-line sed command works just fine.

I understand that you are on a mission to add doublequotes everywhere to appease your personal coding standard. I'm open to adding them where they make a practical difference. I can tolerate adding them in superfluous places for consistency. But I don't want a separate standalone script for doing replacements where the current sed oneliners work just fine. (Yes I know you can come up with some theoretical issue with surprising characters in MSVCVER and SDKVER, but that's clearly out of scope here, and I'm not adding replacevars.py for taking care of that.)

mstorsjo avatar Feb 13 '22 22:02 mstorsjo

Oh, sorry, I somehow totally missed that comment and only saw the inline comments. Would you instead of the mixture of bash, Python, and Perl scripts perhaps prefer just a Python script to do the job of install.sh?

I understand that you are on a mission to add doublequotes everywhere to appease your personal coding standard.

I'm not. I just mechanically did when fixing the issue. But I'm happy to adopt any coding standard of someone else's project when I'm aware of it.

panzi avatar Feb 14 '22 00:02 panzi

Oh, sorry, I somehow totally missed that comment and only saw the inline comments. Would you instead of the mixture of bash, Python, and Perl scripts perhaps prefer just a Python script to do the job of install.sh?

No don't feel the desire to rewrite these scripts in another language at the moment. I don't mind the existing setup at all (I hadn't even reflected on the fact that it does indeed contain a couple separate perl scripts). Just keep this PR as it is, but remove the new replacevars.py script and restore the original calls to sed (possibly with extra quoting of the regexes if you prefer that), and this should be fine with me.

mstorsjo avatar Feb 14 '22 08:02 mstorsjo

Oh, sorry, I somehow totally missed that comment and only saw the inline comments. Would you instead of the mixture of bash, Python, and Perl scripts perhaps prefer just a Python script to do the job of install.sh?

No don't feel the desire to rewrite these scripts in another language at the moment. I don't mind the existing setup at all (I hadn't even reflected on the fact that it does indeed contain a couple separate perl scripts). Just keep this PR as it is, but remove the new replacevars.py script and restore the original calls to sed (possibly with extra quoting of the regexes if you prefer that), and this should be fine with me.

Ping - I'd be fine with the rest of this PR if you'd skip the rewrite/addition of a new script and just keep this to adding more quoting.

mstorsjo avatar Mar 09 '22 13:03 mstorsjo