[PATCH] D144179: [Clang] Added functionality to provide config file name via env variable

Wed Mar 8 13:36:29 PST 2023

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

In D144179#4166909 <https://reviews.llvm.org/D144179#4166909>, @mgabka wrote:

> In D144179#4146599 <https://reviews.llvm.org/D144179#4146599>, @MaskRay wrote:
>
>> This looks like introducing a footgun (when something behaves differently from an upstream Clang, it would be difficult for toolchain maintainers to know why).
>> Why can't your user specify `CLANG_CONFIG_FILE_SYSTEM_DIR`?
>
> hi @MaskRay,
> The reason we wanted to suggest use of environment variable is that the CLANG_CONFIG_FILE_SYSTEM_DIR is only defined at compilation time, after discussing it once again we would rather lean towards introducing an environment variable with similar semantics as CLANG_CONFIG_FILE_SYSTEM_DIR or rather ``CLANG_CONFIG_FILE_USER_DIR``, the motivation here is that it will allow to specify the directory to search for config files in a dynamic way, without need to recompile the compiler.
> It is for user convenience in situations when they are using a system wide installation in a location where they do not have access right, and the ``CLANG_CONFIG_FILE_SYSTEM_DIR`` and ``CLANG_CONFIG_FILE_USER_DIR`` were not defined at build time.
>
> We realised that environment variables are already used in this area, for example CLANG_NO_DEFAULT_CONFIG, so adding another one is not breaking existing convention.
>
> What do you think about it?

Environment variables are evil and make troubleshooting difficult. `CLANG_NO_DEFAULT_CONFIG` disables configuration files and makes the behavior is less magical, so I am fine with it.
It's different from the environment variable being introduced in this patch.

If your users want to use a system installed clang with an environment variable, why not give them a Clang wrapper like:

  #!/bin/zsh
  exec path/to/clang ${CONFIG_FILE:+--config=$CONFIG_FILE "$@"

You can even replace the current `clang` symlink with the wrapper.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144179