Skip to content

[FIX]: Libraries shadowing #2866

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
Merged
merged 10 commits into from
Jul 24, 2023
Merged

[FIX]: Libraries shadowing #2866

merged 10 commits into from
Jul 24, 2023

Conversation

imLinguin
Copy link
Member

fixes shadowing of some libraries. Thanks @Etaash-mathamsetty for finding this bug

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@imLinguin
chore: update nile
9d906a9 
fixes shadowing of some libraries
@imLinguin imLinguin requested review from a team, arielj, flavioislima, CommandMC and Nocccer and removed request for a team July 16, 2023 18:54
@Etaash-mathamsetty
Copy link
Member

The issue is not fully fixed, but at least WRC works now :)
mangohud still doesn't work unfortunately

@imLinguin imLinguin changed the title [Chore]: update nile [FIX]: Libraries shadowing Jul 17, 2023
@imLinguin imLinguin added the pr:testing This PR is in testing, don't merge. label Jul 17, 2023
@imLinguin
Copy link
Member Author

This PR should also fix issues with Steam runtime in Heroic flatpak

imLinguin added 4 commits July 18, 2023 17:47
@imLinguin
fix: native detection for settings
b71e71f
@imLinguin
fix: wrappers for native games
9710432 
additionally added STEAM_COMPAT_INSTALL_PATH variable which shows install location of a game for compatibility tools
@imLinguin
chore: update gogdl
c1b1203
@imLinguin
fix: use given_name to avoid displaying user's last name
8c42bdf
@imLinguin imLinguin added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:testing This PR is in testing, don't merge. labels Jul 22, 2023
@arielj
Copy link
Collaborator

arielj commented Jul 23, 2023

I'm not sure how to review this one cause I don't understand what's the issue it's fixing, can you provide more context in the description @imLinguin ?

cause I also see the branch is named update-nile but it's doing other things

@imLinguin
Copy link
Member Author

This solves multiple issues with gogdl and nile shadowing the libraries of subprocesses. It also moves all wrappers to be added into --wrapper parameter, instead wapping our tools in them (which solves some issues with Steam runtimes in Flatpak too)

Added Nile to gamesdb mapping, fixed how settings detect native game (they checked if native version is available).

@arielj
Copy link
Collaborator

arielj commented Jul 23, 2023

got it, I understand the update for gogdl and nile are new binaries supporting the --wrapper option, right?

changes look good to me, at least what I understand of the code, I've never had the issue so I'm not sure it to actually test a before/after but I trust you and @Etaash-mathamsetty tested this already and works

maybe somebody else with a better understanding about wrappers can also review the code

arielj
arielj reviewed Jul 23, 2023
View reviewed changes
src/backend/config.ts
wineVersion: defaultWine
wineVersion: defaultWine,
enableEsync: true,
enableFsync: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is changing the default to be true right? is this intentional?

I'm fine with that, just want to be sure this was the idea

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I find it weird that we didn't have esync and fsync as default

@imLinguin
Copy link
Member Author

Yes, there are also improvements for removing part from LD_LIBRARY_PATH which is added by pyinstaller (essentially solution for library shadowing)

@arielj arielj added this to the 2.9 milestone Jul 23, 2023
flavioislima
flavioislima approved these changes Jul 23, 2023
View reviewed changes
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

LGTM

@flavioislima
Copy link
Member

It is working fine, although now you have conflicts to fix 😢

@imLinguin
Copy link
Member Author

will solve them tomorrow. Then I'll also update nile to handle being offline, it bails out currently (story for another PR)

@flavioislima
Copy link
Member

will solve them tomorrow. Then I'll also update nile to handle being offline, it bails out currently (story for another PR)

Sure, no problem 👍🏽

imLinguin added 2 commits July 24, 2023 13:30
@imLinguin
Merge branch 'main' of github.com:Heroic-Games-Launcher/HeroicGamesLa…
034729d 
…uncher into update-nile
@imLinguin
chore: update nile
069a7ac 
update contains following commits

fix: do not attempt to refresh token when launching the game
feat: detect native scummvm, replace backslashes in command
@flavioislima flavioislima merged commit 53b5bcb into main Jul 24, 2023
@flavioislima flavioislima deleted the update-nile branch July 24, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arielj arielj arielj left review comments

@flavioislima flavioislima flavioislima approved these changes

@CommandMC CommandMC Awaiting requested review from CommandMC CommandMC was automatically assigned from Heroic-Games-Launcher/development

@Nocccer Nocccer Awaiting requested review from Nocccer Nocccer was automatically assigned from Heroic-Games-Launcher/development

Assignees
No one assigned
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Milestone
2.9
Development

Successfully merging this pull request may close these issues.
4 participants
@imLinguin @Etaash-mathamsetty @arielj @flavioislima