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

PauseMenuScripting: resolve absolute 'builtin' path before substring check #15720

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented Jan 27, 2025

In 99% of the cases, this behaviour is identical to before. With this commit, it is again possible to have 'builtin' a symlink that e.g. points to the engine source directory, which is helpful for development purposes.

This was changed in eeb6cab and immediately broke my build setup. Hence this PR.

I also think that getClient()->getBuiltinLuaPath() is safer, as RUN_IN_PLACE=1 builds might have path_user and path_share in the same place, which can result in erroneously picking the wrong path [during development].

To do

This PR is Ready for Review.

How to test

  1. Read the code
  2. The settings menu must work

…check

In 99% of the cases, this behaviour is identical to before.
With this commit, it is again possible to have 'builtin' a symlink that e.g.
points to the engine source directory, which is helpful for development purposes.
@SmallJoker SmallJoker added the Bugfix 🐛 PRs that fix a bug label Jan 27, 2025
src/script/scripting_pause_menu.cpp Outdated Show resolved Hide resolved
src/script/scripting_pause_menu.cpp Outdated Show resolved Hide resolved
src/script/scripting_pause_menu.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

didn't test the symlink case but it works

@sfan5 sfan5 added this to the 5.11.0 milestone Jan 28, 2025
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 30, 2025
@SmallJoker SmallJoker requested a review from grorp February 1, 2025 08:32
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

LGTM

@grorp grorp added >= Two approvals ✅ ✅ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) One approval ✅ ◻️ labels Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants