Conversation
|
Please note that the naming is proposed; the existence of this PR should not be taken as an indicator that my mind is made up and I will not budge. 🙂 Also, PLMK if the documentation seems adequate/correct. (Though we can always fix that editorially if needed.) |
|
I think I understand the intention of |
|
@dcbaker , thanks for the review! I see your point, and |
|
+1 for the names and general schema change. The wording has room for improvement. While I'm happy with the names as fine, I think avoiding the discussion of |
|
To be clear: description text is not a blocker for me |
I don't. To the extent this is also a runtime requirement, it definitely isn't limited to |
|
There's no linker besides It's not a runtime requirement at all. That's what normal |
|
The CMake documentation is for users of CMake, the CPS documentation is for build system implementors. It behooves us to be more technically specific about what fields are for in the implementation of build systems. |
|
Meson doesn't have a public interface for setting -rpath-link, but we do it automatically for bfd (I think we do it for gold too even though it isn't necessary, but I digress). I agree with @nickelpro that adding notes about implementation details is appropriate, even if we want to couch them in an "implementation details" block or some such. |
Clarify that the list of libraries supplied by `link_libraries` is a list of file paths, not a list of component references.
Add an attribute to allow specifying requirements that are only needed to satisfy the dynamic library loader (or the linker, when operating in certain strict modes). Because these are only needed by the build in limited cases (and arguably are only "needed" to work around broken environments), this is not being treated as a breaking schema change. While this could conceivably have a `dyld_libraries` companion, that is not being added at this time. (Anyway, `link_libraries` is discouraged, and `dyld_libraries` would be as well.) Fixes: cps-org#106
...and because it doesn't, trying to run the resulting executable will also fall flat on its face. If we have A, which (privately) links B... we need this in the case that B can't otherwise be found. If B is in A's |
I'd be okay with this. Actually, I can think of other reasons we might want it, e.g. nothing talks about how the various |
|
@memsharded, @nickelpro, if you're okay with the current state, can one of you actually approve this? (A "+1" comment doesn't count; please leave a Review.) |
It does need us to hold its hand, BFD does not expand That said I don't have any actual problems with the PR. |
|
For posterity:
This knowledge is out-of-date, it turns out this was fixed in binutils 2.28, so this field is barely needed anymore, and is mostly a legacy novelty. |
memsharded
left a comment
There was a problem hiding this comment.
Good for me, sorry I am late to the review.
Approved from my side.
|
|
||
| Specifies a list of additional libraries that must be linked against | ||
| when linking code that consumes the component. | ||
| Specifies a list of additional libraries (as paths, not components) |
There was a problem hiding this comment.
It would be good to clarify here that the paths doesn't need to be absolute, and that a possible intended use case, if I understood it correctly, is linkage to system libraries such as typical -lm -lpthread or w_sock32 in Win, that are necessary to be specified, so link_libraries = ["wsock32"] would be a valid entry?
| shall have precedence. | ||
|
|
||
| .. ---------------------------------------------------------------------------- | ||
| .. cps:attribute:: dyld_requires |
There was a problem hiding this comment.
Good to me, though would have been nice to have a explicit reference to the -rpath-link ld flag as a motivator and example of the need for this item.
Add attributes to allow specifying requirements that are only needed to satisfy the dynamic library loader (or the linker, when operating in certain strict modes).
Because these are only needed by the build in limited cases (and arguably are only "needed" to work around broken environments), this is not being treated as a breaking schema change.
Fixes: #106