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.

Sign up for GitHub

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

Jump to bottom

fix: handle parameter spread for lts version #496

Merged
merged 2 commits into from
Nov 13, 2022
Merged

fix: handle parameter spread for lts version #496

merged 2 commits into from
Nov 13, 2022

Conversation

Danielduel
Copy link

@Danielduel Danielduel commented Oct 9, 2022 •
edited
Loading

Description

Allows the modded game to be ran with parameters from launcher level.
basically it allows to do something like this:
/bin/sh "/pat/to/game/directory/run_bepinex.sh" "" "-screen-width 1000" after setting cwd to game directory.
... this "" works like skipping argument for game executable AND doesn't appear in $@ from game point of view

Motivation and Context

I am rewriting CastleStory launcher and figured out that this is most likely a mistake in run_bepinex, I don't want to update to 6.x yet - it is fixed there tho (at least looks like so).

How Has This Been Tested?

I've just run the game from command line and by spawning process from launcher.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • (I guess?) New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • [?] My change requires a change to the documentation.
  • [?] I have updated the documentation accordingly.

(I can update docs if it is needed ^^)

@bb010g
Copy link
Collaborator

bb010g commented Oct 11, 2022

Please double quote $@. If you need to shift to make that work, that's fine, but don't expand unquoted variables.

@Danielduel
Copy link
Author

Thank you, I didn't thought about that. I will push the fix later today.

@Danielduel
Copy link
Author

This looks interesting as additional safety measure on top of quoting
(sfw) https://dev.to/greymd/why-1-is-used-in-shell-script-364h

@Danielduel
Copy link
Author

Danielduel commented Oct 11, 2022 •
edited
Loading

For readability sake I've sticked to "$@" instead of what this dev.to article suggests. (+in the article they say that it solves issues that are long gone)
On my side - I had to change the call to be more like
spawn(binsh, [ pathToScript, "", "-paramname", paramvalue ]);
Instead of passing everything in one string :)
Tested with and without params, in both - terminal and launcher.

cc @bb010g

@bb010g
Copy link
Collaborator

bb010g commented Oct 11, 2022

Thanks! If that's all that's needed to make your usage work, that's great. https://mywiki.wooledge.org/BashFAQ and https://mywiki.wooledge.org/BashPitfalls are wonderful resources when writing shell, by the way.

@Danielduel
Copy link
Author

👀 Oh, it will help me, thanks for the recommendation 🎉
For me - ready to merge, but don't have perms to click "the button" ofc ^^

@Danielduel
Copy link
Author

Hi @bb010g - do you know who I would have to ping to get that merged?

@ghorsington ghorsington merged commit 3618c46 into BepInEx:v5-lts Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bb010g bb010g bb010g approved these changes

@ghorsington ghorsington ghorsington approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Danielduel @bb010g @ghorsington