runcommand.sh: do not replace %ROM% tokens when empty

[twojstaryzdomu]

%ROM% appears to be quoted for no reason when empty.

I'm creating custom ports as follows:

addPort "${md_id}-${part}" "${md_id}-${part}" "Default game ${part}" "pushd $datadir; $bindir/binary %ROM%; popd"
...
addPort "${md_id}-${part}" "${md_id}-${part}" "Game ${part} level ${file}" "pushd $datadir; $bindir/binary %ROM%; popd" "$file"

The binary is treating the quoting as input and fails to run. runcommand.log output:

pushd <datadir> /usr/bin/binary ""; popd
<datadir> /
error loading file ""

If there isn't a ROM provided, it makes no sense to quote it.

[cmitu]

I think you're calling the addPort without the needed parameters. You need to supply the 5th parameter with the value that will substitute %ROM% in the emulator command line (4th parameter), see

RetroPie-Setup/scriptmodules/helpers.sh

Lines 1399 to 1414 in b0759ff

	## @fn addPort()
	## @param id id of the module / command
	## @param port name of the port
	## @param name display name for the launch script
	## @param cmd commandline to launch
	## @param game rom/game parameter (optional)
	## @brief Adds a port to the emulationstation ports menu.
	## @details Adds an emulators.cfg entry as with addSystem but also creates a launch script in `$datadir/ports/$name.sh`.
	##
	## Can also optionally take a game parameter which can be used to create multiple launch
	## scripts for different games using the same engine - eg for quake
	##
	## addPort "lr-tyrquake" "quake" "Quake" "$emudir/retroarch/bin/retroarch -L $md_inst/tyrquake_libretro.so --config $md_conf_root/quake/retroarch.cfg %ROM%" "$romdir/ports/quake/id1/pak0.pak"
	## addPort "lr-tyrquake" "quake" "Quake Mission Pack 2 (rogue)" "$emudir/retroarch/bin/retroarch -L $md_inst/tyrquake_libretro.so --config $md_conf_root/quake/retroarch.cfg %ROM%" "$romdir/ports/quake/id1/rogue/pak0.pak"
	##
	## Would add an entry in $configdir/ports/quake/emulators.cfg for lr-tyrquake (setting it to default if no default set)

.

Withouth supplying the parameter, the runcommand line ran by EmulationStation line gets transformed to

"/opt/retropie/supplementary/runcommand/runcommand.sh" 0 _PORT_ "$port_cmd" ""

hence the empty '%ROM%'.

[twojstaryzdomu]

The rom parameter is clearly optional:

@param game rom/game parameter (optional)

The issue isn't in how the addPort function in helpers.sh sets up a port script but rather how runcommand.sh handles empty parameters.

Did you see #3551? It addresses the problem correctly. That said, I don't see why addPort couldn't suppress empty parameters as well from being written to a port script. Quoting them is what has caused the problem here.

Either way, this is clearly a bug. Know of any game binaries that explicitly require an empty place-holder parameter? I don't.

[cmitu]

The rom parameter is clearly optional:

@param game rom/game parameter (optional)

Yes, but if your emulator command references it, it must be set, otherwise you'd get an empty parameter when you start runcommand. Look at the current usages of addPort - the optional parameter isn't used when there's no %ROM% present in the emulator command.

The issue isn't in how the addPort function in helpers.sh sets up a port script but rather how runcommand.sh handles empty parameters.

It's part of the issue - runcommand clearly doesn't expect an empty ROM parameter but the addPort caller is responsible for using it correctly. My choice would be for addPort doing the check and throwing an error when called incorrectly, but I see 2 issues with this:

• addPort is called during configure when the package is installed, so throwing an error here would leave the installation incomplete
• issues with addPort should be detected during scriptmodule development and corrected then, instead of relying on addPort throwing an error.

[twojstaryzdomu]

runcommand clearly doesn't expect an empty ROM parameter

runcommand doesn't need to expect an empty ROM, the other way around, if it encounters a defined parameter it acts on it, replacing %ROM% and quoting it.
That is precisely the idea for my fix. The change is it leaves open the possibility not to use an optional rom and run the binary without it.

addPort may as well handle it as it stated in it definition - as an optional parameter, without any additional conditions.

My PR fixes the bug in your code in an elegant way, enabling users to forgo the rom parameter, and allowing the emulator binary to start a default ROM rather than define it explicitly.

The small code change is opening MORE possibilities for users whose emulator or game has a default rather than restricting users unnecessarily. I don't frankly see any value in throwing errors about when there is a reasonable case for not providing a parameter like here.

[cmitu]

That is precisely the idea for my fix. The change is it leaves open the possibility not to use an optional rom and run the binary without it.

If you don't need the %ROM% parameter, then don't add it to the command line of the emulator in addPort - that's the idea behind it being optional.

My PR fixes the bug in your code in an elegant way, enabling users to forgo the rom parameter, and allowing the emulator binary to start a default ROM rather than define it explicitly.

The small code change is opening MORE possibilities for users whose emulator or game has a default rather than restricting users unnecessarily. I don't frankly see any value in throwing errors about when there is a reasonable case for not providing a parameter like here.

How exactly is this opening 'more' posibilities ? Not sure what 'restriction' you're referring to.

[twojstaryzdomu]

If you don't need the %ROM% parameter, then don't add it to the command line of the emulator in addPort - that's the idea behind it being optional.

On the contrary. It is needed in this case.

How exactly is this opening 'more' posibilities ? Not sure what 'restriction' you're referring to.

The possibility to forgo specifying the rom parameter for any game/emulator that actually treats a rom parameter as optional and allows for two modes of operation:

1. Without a parameter
2. With a parameter

So far so good? runcommand is currently limited to item 2, necessitating a parameter to be provided or feeding an empty "" to the emulator/game binary. I don't think you'll find many emulators or games that handle such empty parameters. My fix enables handling both 1 & 2 cases correctly.

As it stands, the rom parameter in the current runcommand.sh code doesn't pass the test of optional since it feeds empty quoted parameter to a binary ("") if no rom parameter is given. This is a bug if the word optional is taken at its face value.

And since rom is optional, an emulator/game may run without one just as well. My fix enables that possibility for many games that allow specifying an optional level or a mode of operation via a parameter, i.e. %ROM$ in your RetroPie parlance as well as run fine without one.

[cmitu]

The possibility to forgo specifying the rom parameter for any game/emulator that actually treats a rom parameter as optional and allows for two modes of operation:

This is already possible - almost all addPort usages in the ports scripmodules don't set %ROM% in their commands.

[twojstaryzdomu]

This is already possible - almost all addPort usages in the ports scripmodules don't set %ROM% in their commands.

What isn't possible is having %ROM% to enable the use of an optional (just in case this word isn't clear enough: optional - one that may be specified but isn't required) rom path.

My fix enables a possibility of having no parameter supplied as rom to runcommand leaving ROM% just in case for a possible choice of a level/game/rom etc. when it is supplied.

Current state: runcommand supports 1 option with %ROM%, forcing empty quoting when no rom parameter is supplied. This is a bug.
Intended state: runcommand supports 2 options with %ROM%:

1. No parameter mode - nothing gets subsitiuted in place of %ROM%. Binary runs with its default choice. NO EMPTY QUOTING IS SUBSITITUTED FOR %ROM%.
2. Parameter provided - binary is run with rom in place of %ROM.

I hope it's clear what the intent is here for games run with or without a parameter from a single ports script.

[s1eve-mcdichae1]

@twojstaryzdomu would this work?

addPort "${md_id}-${part}" "${md_id}-${part}" "Default game ${part}" "pushd $datadir; $bindir/binary; popd"
...
addPort "${md_id}-${part}-level" "${md_id}-${part}" "Game ${part} level ${file}" "pushd $datadir; $bindir/binary %ROM%; popd" "$file"

Note the different 1st args making two separate "emulator" commands, and the first one doesn't use a %ROM% param in the command.

[twojstaryzdomu]

@s1eve-mcdichae1 it did occur to me to try it but it doesn't work, you'd be duplicating functions. Each port requires its own install/build/configure etc. functions. Why have two ports when you can have one with one set of control functiions? That is far easier to maintain. Accepts a parameter if you supply it and ignores one if you don't. Simple.

My solution serves to avoid many other problems. It doesn't break any existing port that I know of. Why the PR hasn't been accepted I can only speculate. Maintainers here are clearly reluctant to accept contributions to the core scripts.