-
Notifications
You must be signed in to change notification settings - Fork 1
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
improve weak link handling #3
Conversation
|d00 |:white_check_mark:|:white_check_mark:|:x:RTE | 2| | ||
|d01 |:white_check_mark:|:white_check_mark:|:white_check_mark:| 5| | ||
|d10 |:white_check_mark:|:white_check_mark:|:x:RTE | 2| | ||
|d11 |:white_check_mark:|:white_check_mark:|:white_check_mark:| 7| | ||
|
||
###### OSX (XCODE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify the compiler used, and version.
How does "--unresolved-symbols=ignore-all" relate to the ENABLE_EXPORTS executable property [1]? I have found the generated project to be overly verbose for a simple test case. To simplify the generated test project consider: replacing the set/get with a single count_incr, which returns the value, doing away with the header files and just specifying "extern func" in the c files. Your table nicely summarized the results, I think it's as I expect. It took me a little bit to decode the "s00" etc. I am not sure if wearing linking executables needs to be considering, although it was an interesting experiment. Are you considering MODULES to be the same as SHARED for the weak-link checking? [1] https://cmake.org/cmake/help/v3.0/prop_tgt/ENABLE_EXPORTS.html |
I'm pretty sure the two are not related. This argument is only needed to convince GNU ld to leave symbols unresolved in an executable (it's fine with leaving them unresolved for shared libraries).
Good point, I've added an update that tries to cut down on the code.
I'm of the opinion that a lack of apparent use cases shouldn't be sole justification for leaving something out. Sure, it's applicability is crippled on Linux, but on OSX, it seems to get along just fine. Since we've already worked out a sensible MO for handling this case, I don't see a point in going out of our way to remove it, now.
No, not for the internal test project. For the table that is created, the test only considers STATIC and SHARED libs (L). The module (M) is always a MODULE library. For the internal test project in the CMake module, every combination of SHARED,STATIC,MODULE, and EXE target is tested separately for linking against SHARED or STATIC libraries (the tests are cached and are only performed on-demand). The exact configuration of the test project is adjusted to test the particular combination at hand. For example, for an EXE target weak linking against a STATIC lib, the links are [1]:
In this case, the target is the EXE. Whereas to test a SHARED target, the module is made a shared library:
And for the typical MODULE target:
[1]
|
LGTM! |
Here, shouldn't |
Improves the weak-linking CMake module proposed in scikit-build/scikit-build#47.
Also updates the test script output and expected output table in the README.
HAS_DYNAMIC_LOOKUP_<ABC>_<XYZ>
andDYNAMIC_LOOKUP_FLAGS_<ABC>_<XYZ>
variables are pre-cleared and must be set manually.