[Lldb-commits] [PATCH] D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.


================
Comment at: CMakeLists.txt:403
+set(LLVM_CODESIGNING_IDENTITY "" CACHE STRING
+  "Sign executables and dylibs with the given identity or skip if empty (Darwin Only)")
+
----------------
Identity should be a string right?


================
Comment at: cmake/modules/AddLLVM.cmake:795
 
-  llvm_codesign(${name})
+  llvm_codesign(TARGET ${name} ENTITLEMENTS ${ARG_ENTITLEMENTS} FORCE)
 endmacro(add_llvm_executable name)
----------------
sgraenitz wrote:
> beanz wrote:
> > sgraenitz wrote:
> > > Would we want to pass `FORCE` to `add_llvm_executable` conditionally?
> > I'm trying to think about the situations in which we need the `FORCE` option. Since this is connecting as a post-build event it shouldn't be running unless the target re-generates the output, so I'm not sure I understand why we ever need it.
> > 
> > I did the git blame walk back to when the code was initially added in `49dd98a03a`, and there is no explanation. I suspect debugserver doesn't actually need the `--force` option because the author of the initial patch probably hit errors when re-signing the pre-built binary in his build directory.
> > 
> > Thoughts?
> I think you are right, it shouldn't be necessary. In practice, though, I can imagine situations when we want to make sure this won't fail in any case.
> 
> The options are: remove entirely (most correct) OR allow enable per target (most flexible) OR allow enable globally.
> 
> What about the last one? I could add `LLVM_CODESIGNING_FORCE` which is OFF by default. In failsafe/debugging situations it could be turned ON globally. I could remove the `FORCE` parameter here and check the flag in `llvm_codesign` (similar to `LLVM_CODESIGNING_IDENTITY`).
Patch updated


Repository:
  rL LLVM

https://reviews.llvm.org/D54443