Skip to content

Conversation

@eerii
Copy link

@eerii eerii commented Nov 14, 2024

When linking libraries with many dependencies, the number of linking arguments can be so big that it exceeds ARG_MAX and cause linking to fail. For example, this came up when linking gstreamer with some plugins.

In reality, many of the arguments get repeated across the crates, so using deduping can help greatly with this issue. This needs to be done with care, since the order of the arguments matter. system_deps already had functions for getting every collection of unique linking arguments in tests, so we use those if SYSTEM_DEPS_CLEAN_LINKER is set. While in my tests everything seems to link fine using this order, I don't think it is wise to do it by default since there may be edge cases where this isn't true.

Additionally, since the two were very intertwined, this PR also includes the option SYSTEM_DEPS_LINK=static_release. This allows to link dependencies dynamically on debug and statically on release.

These changes should not modify existing behaviour, but they do modify the public API since Dependencies::all_libs has a different return type. If needed, I could leave this function as is and create a all_libs_with_static, but it seemed redundant.

TODO:

  • Add tests
  • Link related PRs

Note: This was originally part of the PR for supporting prebuilt binaries (ADD LINK). However, in an effort to make reviewing easy, I separated this feature since it works independently.

@gdesmott
Copy link
Owner

so we use those if SYSTEM_DEPS_CLEAN_LINKER is set

That seems a bit hacky. Could we find a safe way to deduplicate, even partially?

@xclaesse @nirbheek any suggestion? Does meson has a smarter way to deal with this problem?

@eerii
Copy link
Author

eerii commented Nov 14, 2024

I added that because I'm not sure if the deduplicated ordering is always correct, but I agree that it would be better if it did it by default with guarantees. My intuition is that since the relative ordering of the items (libs, frameworks, args, ...) is maintained for the items of each individual library it should work, but I'm not an expert on linking order so I would love to get some feedback on that.

@nirbheek
Copy link

You should copy what Meson is doing for de-duplicating arguments. You don't need to copy all of it, just the code that de-dups linker args. The basic mechanism for that is:

  1. Resolve -L -l args to on-disk files (absolute paths)
  2. If you can't find a file corresponding to a linker line, keep the -L -l args as-is
    • This can happen for args like -lm which may not have an on-disk file at all, or if the on-disk file will be available at build time, and isn't available right now (for whatever reason)
  3. Always de-dup absolute paths in the linker line

@nirbheek
Copy link

@eerii
Copy link
Author

eerii commented Nov 14, 2024

Thank you for all of the references! I'll try this so hopefully we can ship it as the default. For arguments other than -L and -l (for example, includes -I and defines -D), I assume it is safe to deduplicate them as it's done now? Or should the same logic be applied to them?

@nirbheek
Copy link

I haven't looked in detail at this PR, but de-duplication of -I and -D has to be done in the opposite manner (to each other).

  • -I args that come first override those that come later (similar to -L)
  • -D args that come later override those that come earlier.

You also need to account for -U args when de-duplicating -D args. The two links regarding CompilerArgs I pasted above will help you with all this.

If the scope spirals out of control, I would recommend keeping this to -I and -L -l, since -D and -U are rarely duplicated and shouldn't contribute explosively to the argument list. The reason why Meson de-duplicates so aggressively, is because it helped in keeping the build.ninja file small, which helped with speeding up meson setup since writing it out was a significant portion of the time.

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