please icon indicating copy to clipboard operation
please copied to clipboard

Spaces in root path cause plugins to fail

Open cemeceme opened this issue 1 year ago • 3 comments

If the current project being built contains spaces and it uses a plugin, plugin_repo will fail, as it cannot locate arcat. i.e. if the project resides in /dir/with spaces/proj will fail, as /dir/with will get used instead of the full path to the tool.

This appears to be related to $TOOL not being in quotes on this line. This also goes for all 3 instances of the use of $TOOL in the subrepo_rules.build_defs file (Here, here, here).

While just adding quotes around each $TOOL fixes this issue, I'm starting to wonder if a more universal solution could be offered by please? It looks like just passing quoted values into variables is not feasible (as per this issue), but some sort of mechanism to wrap strings like this properly with please would be very useful.

cemeceme avatar Jul 18 '23 16:07 cemeceme

Yeah, this is kind of a known issue. I don't have any major ideas. Using spaces around $TOOL might work, but it isn't going to work for e.g. $TOOLS where the spaces are used to separate the tool paths. I don't have any great ideas, however to suggest that you avoid using filepaths with spaces.

Open to suggestion (or better yet, a PR) though

Tatskaari avatar Jul 18 '23 16:07 Tatskaari

One solution would be to use bash array and associative arrays, maybe please could generate a 'pre-script' containing

declare -a TOOLS=("/dir/with spaces/proj" "/dir2")
# For named args
declare -A SRCS=( [FA]=./filea [FB]=./fileb )

Then the syntax differ to access the values in the command

echo "${TOOLS[0]} ${SRCS[FB]}"

Of course this only works with bash version 4+

izissise avatar Jul 25 '23 15:07 izissise

I was thinking more about this issue, and I was wondering: Wouldnt it work if please did the string substitutions instead? For instance, after passing in the cmd argument for build_rule, please could just substitute out variables like $TOOL and $SRCS etc. It should have enough information before spinning up a shell to do so.

Now, in order to keep backwards compatibility, or to make sure this substitution can be disabled, the cmd argument for build_rule could accept two types: the current str type that wouldnt do the substitution and a new cmdStr(let me know if you think of a better name for this) type, that is an object produced by a buildCmdStr(str)(again, better names are appreciated) function.

With this, please could use a much saner substitution style (e.g. it appears python uses the {variable} style with .format), that would sidestep having to deal with bash.

I would be open to making a pull request with a prototype, but it might take a while since I havent written any Go before.

cemeceme avatar Nov 28 '23 22:11 cemeceme