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

sdl2-sys: Avoid to add unwanted system folder in library research paths #1208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rofferom
Copy link
Contributor

Default pkg-config-rs behavior is to add default system library paths (ex:
-L/usr/lib/x86_64-linux-gnu).

This is because PKG_CONFIG_ALLOW_SYSTEM_LIBS is set by pkg-config-rs by
default. print_system_libs(false) disable this behavior.

More details in evdev-rs project:
ndesh26/evdev-rs#85

Default pkg-config-rs behavior is to add default system library paths (ex:
-L/usr/lib/x86_64-linux-gnu).

This is because PKG_CONFIG_ALLOW_SYSTEM_LIBS is set by pkg-config-rs by
default. print_system_libs(false) disable this behavior.

More details in evdev-rs project:
ndesh26/evdev-rs#85
@rofferom rofferom force-pushed the pkgconfig-skip-systemlibs branch from 40ccddf to 31b08a4 Compare October 13, 2022 07:25
@Cobrand
Copy link
Member

Cobrand commented Oct 15, 2022

I'm sorry it seems i didn't have the notification a few months ago.

Isn't partially the point of pkg-config to use system libs and find where they are? If you disable that, won't pkg config fail to find the sdl2 paths? I'm having a hard time understanding what would happen to the average user

@rofferom
Copy link
Contributor Author

Actually, gcc has a default research path for system libraries. For the average user, my patch should have no impact at all. On my computer, I get correct flags with :

$ pkg-config --cflags --libs sdl2
-D_REENTRANT -I/usr/include/SDL2 -lSDL2

Instead, the default behavior of Rust pkg-config crate is

$ PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --cflags --libs sdl2
-D_REENTRANT -I/usr/include/SDL2 -L/usr/lib/x86_64-linux-gnu -lSDL2

A redundant -L option is added. Like I mentioned in ndesh26/evdev-rs#85, it is only an issue if you want yo use a custom version of a library in your project.

I think there is no impact for your project, but I would understand if you don't agree with that.

@Cobrand
Copy link
Member

Cobrand commented Oct 15, 2022

Actually, gcc has a default research path for system libraries.

I don't think is this true for static linking, only for dynamic linking. I know the pkg-config feature was introduced partially due to static linking. Could you please test that it still works with your PR?

@rofferom
Copy link
Contributor Author

Actually, gcc has a default research path for system libraries.

This is "true", but we are not using gcc. I made some tests to force a sample C program to statically link with SDL2, there is no issue when system. But when I'm forcing a static link when using rust-sdl2, I have a link issue. So my initial statement is false in the context of this PR :)

I don't think is this true for static linking, only for dynamic linking. I know the pkg-config feature was introduced partially due to static linking

You are right rust-lang/pkg-config-rs#11, there are many complains link mine, but what you said is explained many times. There are also many references of projects that are doing the same kind of change than this PR.

Maybe the correct update of my patch would be

-        .print_system_libs(false)
+        .print_system_libs(statik)

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