Sitelet https://github.com/linuxdeploy/linuxdeploy/pull/188
Skip to content

Possible fix for #149 - #188

Merged
TheAssassin merged 5 commits into
linuxdeploy:masterfrom
pavmk:fix_149
Jan 12, 2022
Merged

TheAssassin merged 5 commits into
linuxdeploy:masterfrom
pavmk:fix_149

Conversation

@pavmk

@pavmk pavmk commented Jan 11, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@TheAssassin

Copy link
Copy Markdown
Member

I think the reason the rpath was not touched if set already in linuxdeployqt is that too many assumptions were made while this was implemented. The only reason linuxdeploy does this this way is because linuxdeployqt does, too. I do not think it makes sense to just ignore the rpath altogether.

We should instead try to add (prepend/append) $ORIGIN to an existing rpath if it doesn't contain the value already. Could you please update your PR to implement such a behavior? I'm not sure prepending makes a lot of sense, I'd rather append, I guess.

@pavmk

pavmk commented Jan 11, 2022

Copy link
Copy Markdown
Contributor Author

Main reason of this PR is to fix gstreamer plugin that is broken now. It's happened, because linuxdeploy override rpath that gstreamer-plugin set. And, maybe, it's a fix for #149.

We should instead try to add (prepend/append) $ORIGIN to an existing rpath if it doesn't contain the value already. Could you please update your PR to implement such a behavior? I'm not sure prepending makes a lot of sense, I'd rather append, I guess.

Done.

Comment thread include/linuxdeploy/util/misc.h Outdated
Comment thread src/core/appdir.cpp Outdated
return false;

d->setElfRPathOperations[sharedLibrary] = "$ORIGIN";
auto rpath = elf_file::ElfFile(sharedLibrary).getRPath();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please make these vars const. Otherwise, this looks really good, thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/core/appdir.cpp
Comment thread src/core/appdir.cpp Outdated
Co-authored-by: TheAssassin <theassassin@assassinate-you.net>
Comment thread include/linuxdeploy/util/misc.h Outdated
Comment thread include/linuxdeploy/util/misc.h Outdated
@TheAssassin
TheAssassin merged commit 4c5b9c5 into linuxdeploy:master Jan 12, 2022
@TheAssassin

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants