[PATCH] D105811: [flang][driver] Delete `f18` (i.e. the old Flang driver)

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/CMakeLists.txt:86
+    message(FATAL_ERROR "Clang is not enabled, but is required for the Flang driver")
   endif()
 endif()
----------------
awarzynski wrote:
> mehdi_amini wrote:
> > A few lines above, we enable MLIR automatically as a dependency of clang instead of erroring out, any reason you didn't just apply the same logic here?
> This came up before. `LLVM_ENABLE_PROJECTS` is a `CACHE` variable (i.e. user variable), so we shouldn't modify it here. In https://reviews.llvm.org/D101842, @Meinersbur  suggested tweaking `LLVM_TOOL_*_BUILD` instead. See also this [[ https://reviews.llvm.org/D101842#inline-964091 | comment ]].
> 
> The lack of consistency in how the MLIR and CLang dependencies are treated is not great, but I've not had the bandwidth to fix this. Do you have a preferred approach here? I'm not a CMake expert - suggestions are very welcome! Btw, apologies for the delay, I was AFK for a couple of weeks.
To reiterate, CACHE variables are considered user-controlled and IMHO should not be modified by the script. Interaction is complicated and the CMake manual warns at several places. What may happen after the initial `set(LLVM_ENABLE_PROJECTS CACHE)`:

 * `set(LLVM_ENABLE_PROJECTS)`: Creates a new variable in the current scope that shadows the CACHE variable. When leaving the scope, the CACHE variable becomes visible again. Note that lines 75 and 81 do exactly this.
 * `set(LLVM_ENABLE_PROJECTS CACHE)`: Ignored, but may or may not remove any shadowing local scope variable again, such as those set at lines 75 and 81 (see [[ https://cmake.org/cmake/help/latest/policy/CMP0126.html#policy:CMP0126 | change in CMake 3.21 ]])
 * `set(LLVM_ENABLE_PROJECTS CACHE FORCE)`: Overwrites the user-set CACHE variable (probably what you expected to happen), possibly also removing the local scope variable.

Re-setting a cached variable may also cause the cmake configure script to run again at `make` after the initial run. Now the value of the cached value has the overwritten value from the beginning, which may change script behaviour before it was overwritten.

I suggested to make better use of `LLVM_TOOL_*_BUILD` instead, whose default value currently is just derived from LLVM_ENABLE_PROJECTS, but could also take dependencies into account. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105811/new/

https://reviews.llvm.org/D105811