This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Added functionality to provide config file name via env variable
AbandonedPublic

Authored by jolanta.jensen on Feb 16 2023, 3:25 AM.

Details

Summary

Also added functionality to set the name of the config file env variable
via CMake.

Diff Detail

Event Timeline

jolanta.jensen created this revision.Feb 16 2023, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 3:25 AM
jolanta.jensen requested review of this revision.Feb 16 2023, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 3:25 AM
MaskRay added a comment.EditedFeb 16 2023, 10:34 AM

Behaviors due to a new environment variable should be very careful. How is this useful? If you want this, you can add a wrapper around clang to specify --config= by yourself.

Think about this: if the user sets CONFIG_FILE= to something, it will be difficult for toolchain maintainers to triage a bug.

Extended the test to cover Windows OS.

Behaviors due to a new environment variable should be very careful. How is this useful? If you want this, you can add a wrapper around clang to specify --config= by yourself.

Think about this: if the user sets CONFIG_FILE= to something, it will be difficult for toolchain maintainers to triage a bug.

The feature is aimed at system administrators rather than an individual user. It simplifies system configuration so the user does not need to configure anything him/herself.

Adjusted the test for Windows OS. Once again.

Adjusting the test for Windows OS again.

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?

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?

Because I don't want the user to be specifying any flags compiling. The feature is intended to be used by a system administrator who will configure the system for the user so no compile flags are necessary. That way the machines can be configured the same way for all users.
Since there were concerns about visibility of what configuration is used, I checked the driver crash report generated when a configuration file set via an environmental variable is used. The crash report will print the configuration file used and the flags specified in the configuration file will be appended to the driver args in the crash reproducer.
And finally, you already use an env variable, CLANG_NO_DEFAULT_CONFIG, to disable use of default configuration so it isn't really an introduction but rather an extension of what is already used.

Adjusting the test for Windows. Setting the read bits only for the config file as Windows does not have full support for chmod.

Test adjustment for Windows. Disabling read permissions using chmod does not seem to work on Windows.

mgabka added a comment.Mar 3 2023, 5:16 AM

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?

MaskRay requested changes to this revision.EditedMar 8 2023, 1:36 PM

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.

This revision now requires changes to proceed.Mar 8 2023, 1:36 PM
jolanta.jensen abandoned this revision.May 25 2023, 8:33 AM