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

libcurl.pc.in: drop LDFLAGS from Libs.private #15533

Closed
wants to merge 1 commit into from

Conversation

petermarko
Copy link

Stop passing linker flags to pkg-config.

This was added in v8.11.0 with commit [1].
There are several problems with this, especially:

  • user may want to link curl and application with different flags
  • user usually adds the same or similar flags in all components, so this will double the flags when linking application
  • when building components in temporary directories, these directories are preserved in pkg-config linker flags and are invalid when building application

[1] 9f56bb6

Stop passing linker flags to pkg-config.

This was added in v8.11.0 with commit [1].
There are several problems with this, especially:
* user may want to link curl and application with different flags
* user usually adds the same or similar flags in all components, so this
  will double the flags when linking application
* when building components in temporary directories, these directories
  are preserved in pkg-config linker flags and are invalid when building
  application

[1] curl@9f56bb6

Signed-off-by: Peter Marko <peter.marko@siemens.com>
@vszakats
Copy link
Member

vszakats commented Nov 9, 2024

LDFLAGS were added in a different commit in an earlier release to libcurl.pc for CMake builds (see 296e855). The flags are libpaths and frameworks, which seems critical.

The commit referenced in OP added them for autotools, to bring the two build methods closer.

Can you give examples of the issues you experienced to understand them better? Are you seeing these with autotools, cmake, both?

@vszakats vszakats added the build label Nov 9, 2024
@petermarko
Copy link
Author

I'm seeting this when building within Yocto project, which uses autotools for curl component.
This is cross-compiling in temporary directory and has a quality check that resulting files cannot contain reference to it
Without this the build fails at this line in libcurl.pc:

Libs.private: -Wl,-O1 -Wl,--hash-style=gnu -Wl,--as-needed -fcanon-prefix-map -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/curl-8.11.0=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/curl-8.11.0=/usr/src/debug/curl/8.11.0 -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/build=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/build=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot= -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot= -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot-native= -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot-native= -Wl,-z,relro,-z,now -L/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot/usr/lib/..//lib -lidn2 -lssl -lcrypto -lssl -lcrypto -lz

@dfandrich
Copy link
Contributor

dfandrich commented Nov 9, 2024 via email

@vszakats
Copy link
Member

vszakats commented Nov 10, 2024

I'm seeting this when building within Yocto project, which uses autotools for curl component.

This is cross-compiling in temporary directory and has a quality check that resulting files cannot contain reference to it

Without this the build fails at this line in libcurl.pc:


Libs.private: -Wl,-O1 -Wl,--hash-style=gnu -Wl,--as-needed -fcanon-prefix-map -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/curl-8.11.0=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/curl-8.11.0=/usr/src/debug/curl/8.11.0 -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/build=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/build=/usr/src/debug/curl/8.11.0 -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot= -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot= -fdebug-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot-native= -fmacro-prefix-map=/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot-native= -Wl,-z,relro,-z,now -L/home/projects/build/work-ccp7/projects/poky/tmp/work/core2-64-poky-linux/curl/8.11.0/recipe-sysroot/usr/lib/..//lib -lidn2 -lssl -lcrypto -lssl -lcrypto -lz

Can't say off-hand if it would or wouldn't improve the .pc file in general but technically it's probably possible to avoid inheriting custom flags (the majority in this particular example), and instead include just those that resulted from curl's dependency detections. Syncing this with cmake even more.

But this would possibly also not satisfy this checker due to the temp libpath. Which in turn is a valid path and can be required in other build-cases.

(One may also expect that custom LDFLAGS are inherited.)

I'm wondering what more could curl do to satisfy a custom checker?

Also, the checker has all the information to strip what it deems unfitting and create the pc file it needs.

@nekopsykose
Copy link

it's possible to put things like -framework CoreFoundation that are known to be critical into a separate variable (e.g. LDFLAGS_PLATFORM, or any name), and use that in the .pc.in and add it to the final ldflags. then the generic user-facing LDFLAGS would not get re-echoed into the .pc , but things that are important would be. afaik that's how most other projects do it.

the curl-config.in has the same issue in it, technically (having LDFLAGS echoed into it for --static), which has been there for much longer than this change in 8.11

@vszakats
Copy link
Member

I made a branch omitting inherited LDFLAGS from libcurl.pc. Right
on the first test bumped into the case of a manually passed -L via
LDFLAGS now isn't passed to libcurl.pc. This is undesired. At the
same time inherited LIBS are continued to passed to it (unless doing
the same treatment for LIBS, but nobody asked for that.)

Perhaps -L options should be excepted and let through to .pc?
Then probably also -framework options? I don't like processing
flag lists manually, though to a certain extent this is already done, so
by itself may not be a fatally bad thing?

@petermarko
Copy link
Author

the curl-config.in has the same issue in it, technically (having LDFLAGS echoed into it for --static), which has been there for much longer than this change in 8.11

Yocto handles this file differently and has to play the path replacement games to avoid multilib conflicts. Additionally it's used from native build, not from cross-compile build as cross-compiled executables are in general considered unsafe for native execution. And we don't use static linking for libcurl afaik.
I'd say that this causes unintentional pass of the QA test and so it lives around with the potentially unwanted linker flags topic.
It is possible that I can play some path overwrite games also with libcurl.pc to pass our QA, but I personally feel that dumping all linker flags into follow-up linking commands is not the best idea (although it may be just my worry without any real-life consequences after seeing out QA error)

made a branch omitting inherited LDFLAGS from libcurl.pc. Right
on the first test bumped into the case of a manually passed -L via
LDFLAGS now isn't passed to libcurl.pc.

I wouldn't expect that reverting this one line would cause test failures (as that was a working implementation for decades until now).
It also did not break automated tests in the Yocto build (which are however only partial due to cross-compile environment).
But the commit which introduced this is large and there are also lot of commits afterwards, so probably something already started using this as a feature.

Well, I don't know what to do with this PR now to finish it to a mergeable state and there are many opened questions in above comments.
Maybe I should have started a discussion instead as it looks like a difficult topic due to curl being configurable in many different ways, although the onliner seems like a good idea at the beginning.
Is there interest from curl maintainers side to continue in this direction?
If you give me pointers, I could try to extend this.

@vszakats
Copy link
Member

vszakats commented Nov 11, 2024

Deleting a macro reference I don't think gets us closer to the desired outcome. It breaks a bunch of critical cases and reverses track on the effort to sync generated config files.

The desired outcome is to have libcurl.pc generated pretty close (ideally: identical) to each other both via autotools and cmake, also matching the generated curl-config.

For this we'd probably need to decide what's useful to include in them.

What keeps me wondering not just in this report but a few others from recently is why are dependent projects using the .private fields even though none appatently want to link to libcurl statically? Aren't .private fields meant for static linking and should not be used for shared linking?

Here's the WIP branch to stop echoing inherited LDFLAGS: #15550

@vszakats
Copy link
Member

#15550 is ready for review and tests.

@petermarko
Copy link
Author

#15550 is superior to this PR, so closing...

@petermarko petermarko closed this Nov 14, 2024
vszakats added a commit that referenced this pull request Nov 14, 2024
`libcurl.pc` `Libs.private` (since 8.11.0, and in `Libs` before 7.20.0)
and `curl-config` `--static-libs` (since 7.17.1, and in `Libs` between
7.7.2-7.25.0). This included all flags inherited from the environment,
in addition to those coming from dependency detections.

To avoid spilling all linker flags inherited from the environment to
the libcurl config files, this patch omits them all, except `-L`, `-F`,
`--library-path=` and `-framework` options, which are still passed.
The rationale for the exceptions is that `LIBS` is passed as-is, and
`LDFLAGS`, `LIBS` are the canonical way to pass custom libs options
to a build. `LIBS` may not work without a matching custom libpath.

This brings autotools behaviour closer to cmake, and `curl-config`
closer to `libcurl.pc`.

Follow-up to 9f56bb6 #14681
Follow-up to 4c8adc8
Reported-by: Peter Marko
Fixes #15533
Closes #15550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants