| elvisangelaccio |
| Ark |
This is a diff split off from D5990, taking the corresponding comments into account.
Re: "use QStandardPaths::findExecutable() to look for "otool"; I don't see why this would be required for otool and not ldd. Also, the otool command needs an additional argument. So we can either do a runtime lookup if the otool command exists (= we're on Mac) or we can test at compile time if we'll be running on Mac. I think the latter is the more appropriate approach.
| Lint Skipped |
| Unit Tests Skipped |
Re: "use QStandardPaths::findExecutable() to look for "otool"; I don't see why this would be required for otool and not ldd. Also, the otool command needs an additional argument. So we can either do a runtime lookup if the otool command exists (= we're on Mac) or we can test at compile time if we'll be running on Mac. I think the latter is the more appropriate approach.
I prefer a runtime check, imho ifdefs should be used only when strictly necessary (= the code wouldn't compile otherwise). And yes, I'm ok if we look also for ldd through findExecutable().
So what if a Linux user has an otool command in his or her path that does something completely unrelated (or a Mac user with ldd)? Those commands are reserved on their respective platforms, but not elsewhere. IMHO making this a runtime check could make sense if the same binary would run on both platforms, but that's not the case ...
Just making sure we cover all bases.
Is otool even available on linux? If a mac user also has ldd that wouldn't be a problem, as long as we put the findExecutable(ldd) in the else branch (so if otool is found, we use it).
Anyway, if you *really* want to make this a compile-time check, then I'd prefer to use cmake magic, something like:
if (APPLE) target_compile_definitions(kerfuffle -DDEPENDENCY_TOOL=otool) target_compile_definitions(kerfuffle -DDEPENDENCY_TOOL_ARGS=-L) else() # define as ldd... endif()
and then in the code just:
dependencyTool.setProgram(QStringLiteral(DEPENDENCY_TOOL));
without ifdefs.
I was indeed thinking of testing for otool and then falling back to ldd. However, 'otool' is really a rather vague name (it serves a whole set of related purposes on Mac). It doesn't exist as such on Linux, nor do I know of any existing tool with the name, but that's also exactly why anyone could write a script for whatever (unrelated) purposes. However unlikely that may sound, it's a possibility and if it strikes it'd be "fun" figuring out what goes wrong from the bug report. I don't care personally, and I don't think it'll affect anyone who uses both platforms so if you don't care I'll just put in the runtime checks.
Anyway, if you *really* want to make this a compile-time check, then I'd prefer to use cmake magic, something like:
if (APPLE) target_compile_definitions(kerfuffle -DDEPENDENCY_TOOL=otool) target_compile_definitions(kerfuffle -DDEPENDENCY_TOOL_ARGS=-L) else() # define as ldd... endif()
I personally think that'd be preferable over runtime checking but it's your call.
I'm fine either way (runtime check or build time check via cmake), I just don't like the ifdef in the code if there is another way of doing the same thing.