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

explore alternatives to setting LIBRARY_PATH #118

Open
2 tasks done
flavorjones opened this issue Oct 10, 2022 · 11 comments · Fixed by #131
Open
2 tasks done

explore alternatives to setting LIBRARY_PATH #118

flavorjones opened this issue Oct 10, 2022 · 11 comments · Fixed by #131

Comments

@flavorjones
Copy link
Owner

flavorjones commented Oct 10, 2022

Description

Currently, mini_portile sets the environment variable LIBRARY_PATH in #activate. This behavior has been present since the first commit by Luis in 2011 (c06b4ab).

This is OK the vast majority of the time, however an interesting and unfortunate edge case has shown up when the target system is using Fedora's alternative pkgconf (more reading).

The problem is this: on Fedora, pkg-config --libs will not report any directories that are present in LIBRARY_PATH, which means that any extconf.rb that relies on a vendored library's .pc file to set paths correctly will fail to find the vendored library.

See sparklemotion/sqlite3-ruby#354 for an example of where this has generated a bug report, and https://gitlab.kitware.com/cmake/cmake/-/issues/22148#note_980802 for a related discussion in a non-Ruby context.

Possible solutions

  1. mini_portile could automatically append_ldflags(lib_path)
  2. mini_portile could stop setting LIBRARY_PATH
  3. mini_portile could do both 1 and 2

I think any of these might be a reasonable solution, but only thorough testing against downstream projects is going to reveal whether we'll be introducing a subtle breaking change.

Next steps

Reproduce this issue in CI:

@kwilczynski
Copy link

@flavorjones, thank you for looking into this!

@eregon, does this impact Nokogiri on Red Hat too?

@flavorjones
Copy link
Owner Author

Nokogiri isn't impacted because its extconf.rb does not read the .pc file for any of the vendored libraries.

Ruby-magic likely is affected (and I would say the same for rcee/packaged_tarball) but I haven't yet attempted to reproduce it in CI to confirm.

@flavorjones
Copy link
Owner Author

(And I've updated the description to be clearer that this only affect projects that are relying on a vendored .pc file to set paths correctly.)

flavorjones added a commit that referenced this issue Sep 9, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
@flavorjones
Copy link
Owner Author

See #131 where I'm working on this.

flavorjones added a commit that referenced this issue Sep 9, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 9, 2023
This probably should have always been the behavior. This works around
the behavior of Fedora's pkgconf described in #118.

I'm a little worried that this might break packages in some subtle
way, so the `set_ldflags` option will turn it off if needed.
@flavorjones
Copy link
Owner Author

flavorjones commented Sep 9, 2023

Note for y'all who are following this issue: the approach I'm taking in #131 is for a new method, MiniPortile#mkmf_config which can a pkg-config file and sets $LDFLAGS and $CFLAGS.

flavorjones added a commit that referenced this issue Sep 10, 2023
If MakeMakefile is loaded, assume we're in an `extconf.rb` and:

- prepend a `-L lib_path` and `-l lib_name` to $LDFLAGS
- prepend `-I include_path` to $CPPFLAGS

This should mean we don't need to use the pkgconf trick in extconf.rb
anymore, which completely avoids the behavior of Fedora's pkgconf
described in #118.

I'm a little worried that this might break packages in some subtle
way, so it can be turned off by passing `mkmf: false` to `MiniPortile#activate`

also:

- fix up the windows paths in the the LDFLAGS env var and the tests
- use assert_includes for better failure messages
- extract public class methods MiniPortile.native_path and .posix_path
flavorjones added a commit that referenced this issue Sep 10, 2023
If MakeMakefile is loaded, assume we're in an `extconf.rb` and:

- prepend a `-L lib_path` and `-l lib_name` to $LDFLAGS
- prepend `-I include_path` to $CPPFLAGS

This should mean we don't need to use the pkgconf trick in extconf.rb
anymore, which completely avoids the behavior of Fedora's pkgconf
described in #118.

I'm a little worried that this might break packages in some subtle
way, so it can be turned off by passing `mkmf: false` to `MiniPortile#activate`

also:

- fix up the windows paths in the the LDFLAGS env var and the tests
- use assert_includes for better failure messages
- extract public class methods MiniPortile.native_path and .posix_path
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 10, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
flavorjones added a commit that referenced this issue Sep 13, 2023
and a local pkgconf file. This should catch the issue with fedora's
pkgconf in #118.
@flavorjones
Copy link
Owner Author

flavorjones commented Sep 13, 2023

Going to create a release candidate with #131 and #133 in it.

@flavorjones flavorjones reopened this Sep 13, 2023
@flavorjones
Copy link
Owner Author

Reopening until I've made a final release that addresses this.

@flavorjones
Copy link
Owner Author

See

which are both green, but currently there is a high risk of pulling in system shared libraries inadvertently. I've got a few options, but need to find the time to finish this work.

@flavorjones
Copy link
Owner Author

Note: the re2 gem worked around shared linking in mudge/re2#93, there are probably lessons in there we can upstream.

@flavorjones
Copy link
Owner Author

Picking this back up again this week after a related problem was raised in sparklemotion/nokogiri#3133

@flavorjones
Copy link
Owner Author

Note that the mkmf_config(static:) call that's in mini_portile 2.8.5 seems to fix the issue with accidentally dynamically linking. Need to fully test it across a few gems but I think it's mostly there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants
@flavorjones @kwilczynski and others