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

Launcher: explicitly handle cli arguments to be passed to the Component #3714

Merged
merged 9 commits into from
Sep 8, 2024
6 changes: 3 additions & 3 deletions CommonClient.py
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ def get_base_parser(description: typing.Optional[str] = None):
return parser


def run_as_textclient():
def run_as_textclient(*args):
class TextContext(CommonContext):
# Text Mode to use !hint and such with games that have no text entry
tags = CommonContext.tags | {"TextOnly"}
Expand Down Expand Up @@ -1033,7 +1033,7 @@ async def main(args):
parser = get_base_parser(description="Gameless Archipelago Client, for text interfacing.")
parser.add_argument('--name', default=None, help="Slot Name to connect as.")
parser.add_argument("url", nargs="?", help="Archipelago connection url")
args = parser.parse_args()
args = parser.parse_args(args)

if args.url:
url = urllib.parse.urlparse(args.url)
Expand All @@ -1051,4 +1051,4 @@ async def main(args):

if __name__ == '__main__':
logging.getLogger().setLevel(logging.INFO) # force log-level to work around log level resetting to WARNING
run_as_textclient()
run_as_textclient(*sys.argv[1:]) # default value for parse_args
21 changes: 17 additions & 4 deletions Launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,15 +322,28 @@ def main(args: Optional[Union[argparse.Namespace, dict]] = None):
init_logging('Launcher')
Utils.freeze_support()
multiprocessing.set_start_method("spawn") # if launched process uses kivy, fork won't work
parser = argparse.ArgumentParser(description='Archipelago Launcher')
parser = argparse.ArgumentParser(
description='Archipelago Launcher',
usage="[-h] [--update_settings] [Patch|Game|Component] [-- component args here]"
)
run_group = parser.add_argument_group("Run")
run_group.add_argument("--update_settings", action="store_true",
help="Update host.yaml and exit.")
run_group.add_argument("Patch|Game|Component", type=str, nargs="?",
help="Pass either a patch file, a generated game or the name of a component to run.")
run_group.add_argument("args", nargs="*",
help="Arguments to pass to component.")
main(parser.parse_args())
run_group.add_argument("--",
help="Arguments to pass to component.",
dest="args", default=[])
args = sys.argv[1:]
if "--" in args:
i = args.index("--")
passthrough_args = args[i+1:]
args = args[:i]
args = parser.parse_args(args)
args.args = passthrough_args
else:
args = parser.parse_args()
Copy link
Member

@NewSoupVi NewSoupVi Aug 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this part necessary? parser.parse_args already seems to understand how to handle -- correctly. This PR seems to work without any of the changes to this part of the code for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i figured out what the issue with this was (after reverting the -- arg in the Parser), it joins args before and after the -- arg together, so this happens

> python launcher.py test1, test2 -- test3 --test4
Namespace(update_settings=False, args=['test2', 'test3', '--test4'], **{'Patch|Game|Component': 'test1,'})

this is not a hill i'm willing to die on though,

tested the rollback on these lines (and the 'args' arg) in source and frozen and the only scenarios that didn't work as expected were negative tests that just had invalid args ignored ex. python launcher.py test test

will commit if you confirm this is good enough:tm: and a worthy downside to get rid of the manual parsing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got the confirmation in discord

main(args)

from worlds.LauncherComponents import processes
for process in processes:
Expand Down
8 changes: 4 additions & 4 deletions worlds/LauncherComponents.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ def __repr__(self):
processes = weakref.WeakSet()


def launch_subprocess(func: Callable, name: str = None):
def launch_subprocess(func: Callable, name: str = None, args: Tuple[str, ...] = ()) -> None:
global processes
import multiprocessing
process = multiprocessing.Process(target=func, name=name)
process = multiprocessing.Process(target=func, name=name, args=args)
process.start()
processes.add(process)

Expand All @@ -78,9 +78,9 @@ def __call__(self, path: str) -> bool:
return False


def launch_textclient():
def launch_textclient(*args):
NewSoupVi marked this conversation as resolved.
Show resolved Hide resolved
import CommonClient
launch_subprocess(CommonClient.run_as_textclient, name="TextClient")
launch_subprocess(CommonClient.run_as_textclient, name="TextClient", args=args)


def _install_apworld(apworld_src: str = "") -> Optional[Tuple[pathlib.Path, pathlib.Path]]:
Expand Down
Loading