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

Revert "tests: use RUNPATH instead of RPATH consistently (#309)" #323

Closed
wants to merge 1 commit into from

Conversation

jnpkrn
Copy link
Contributor

@jnpkrn jnpkrn commented Sep 14, 2018

This reverts commit eeb5e45.

Said change did not undergo standard review and seems superfluous,
since we inject neither RPATH nor RUNPATH into the resulting shared
library, therefore it's questionable why we should be carrying this
burden and influence anything by force. Redistributors are free to
make their own choices. If that requires assistance on libqb's side,
their conditional patches are welcome.

Signed-off-by: Jan Pokorný [email protected]

…#309)"

This reverts commit eeb5e45.

Said change did not undergo standard review and seems superfluous,
since we inject neither RPATH nor RUNPATH into the resulting shared
library, therefore it's questionable why we should be carrying this
burden and influence anything by force.  Redistributors are free to
make their own choices.  If that requires assistance on libqb's side,
their _conditional_ patches are welcome.

Signed-off-by: Jan Pokorný <[email protected]>
@fabbione
Copy link
Member

nack.
RUNPATH is required for the test suite to work properly on some combinations of OS/toolchain, otherwise the test suite can fail or use the OS installed libqb.

@fabbione
Copy link
Member

Also:

  1. read the comment in the original commit properly, it explains why this is necessary to run properly the test suite. How can you define it superfluous if it fixes the test suite?
  2. as mentioned in the original comment, libqb it self was not affected and it is not affected. It is merely to execute the test suite properly
  3. what is the gain of reverting the patch and introduce a regression in the test suite?
  4. the primary re-distributor that has this issue is RHEL and Fedora btw that still defaults to the old linker behavior.
  5. the patch was acked by Chrissie on IRC, given that the code didn't influence any function in the main library we just did a quick turnaround.

@chrissie-c
Copy link
Contributor

NACK. I'm not reverting a change just because you didn't get to review it.

@chrissie-c chrissie-c closed this Sep 17, 2018
@jnpkrn
Copy link
Contributor Author

jnpkrn commented Sep 17, 2018

Misunderstanding emerges.

I am all for retaining that provided that the reason is properly justified.
So far, it appears just as an additional complexity not paralleled elsewhere.
The main purpose of this dispute is to remove the entangled obscurity with
said commit, something not very helpful for an open source project.

Can you answer these simple questions, please, so we may go forward?

  1. Does kronosnet test suite use system-wide libqb or does it consume it
    internally as an subproject, via libtool depending on its *.la files etc.?

  2. Does kronosnet test suite require libqb compiled with any sort of
    dynamic linker search path overrides (*RPATH, *RUNPATH)?

  3. Is there any actual risk libqb would taint intended separation by
    being used to load any libraries transitively? -- this is the only case I can
    imagine would need this rectification.

Please, enlighten me, which configuration in particular is troubling to
the extent said commit is necessary. Thank you.

@jnpkrn jnpkrn reopened this Sep 17, 2018
@fabbione
Copy link
Member

fabbione commented Sep 17, 2018 via email

@fabbione fabbione closed this Sep 17, 2018
@jnpkrn
Copy link
Contributor Author

jnpkrn commented Sep 17, 2018

Please, enlighten me, which configuration in particular is troubling to
the extent said commit is necessary. Thank you.

Your own test suite.

Ah, I start to see, I really wish the initial problem statement was
less obfuscated (which is one of the purposes for patch review).

So does it mean that:

a. libtool on some systems (Debian?) will compile test suite binaries
with RPATH unconditionally and automatically...

b. ...which libtool's wrapper scripts for these binaries will try to
override then with LD_LIBRARY_PATH=... definition, which will fall
short to achieve the override (as only RUNPATH can be overridden?)

Or is the reproducing scenario different?

@fabbione
Copy link
Member

fabbione commented Sep 17, 2018 via email

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.

3 participants