[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai abandoned this revision.
nemanjai added a comment.

Thanks everyone for providing feedback on this. I posted this to gauge interest 
in the community for such a change. As it appears, the consensus seems to be 
that this isn't desired so I will abandon this change and vendors will continue 
with their homegrown solutions to this problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D103125#2782096 , @echristo wrote:

> I'm also not a fan of this change. From a project perspective the binary is 
> clang and while people may wish to change the name for their own product 
> teams it seems like that onus should be on them.

It occurs to me that there's precedent regarding prefixes, which is that you 
can rename/soft-link clang to have a triple embedded in the name (e.g., 
x86_64-pc-linux-clang) and then the driver will factor the prefix into 
default-target computations.  But AFAIK there's no support for directly doing 
that renaming/linking in the build system.  I agree with @echristo this 
properly belongs to the vendors not the project.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

I'm also not a fan of this change. From a project perspective the binary is 
clang and while people may wish to change the name for their own product teams 
it seems like that onus should be on them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D103125#2781239 , @jhenderson 
wrote:

> In D103125#2780936 , @dblaikie 
> wrote:
>
>> Can't say I'm super enthusiastic about this (I assume the build already 
>> supports prefixes and suffixes, which I'd hope would be adequate - but 
>> presumably are not for your use case), though there's some, I think, related 
>> prior art: Sony folks (@probinson @jhenderson) have (or had at some point) 
>> different C++ language standard/version defaults than upstream and have 
>> maintained/made changes to upstream test cases that assume the upstream 
>> default version to not make that assumption (to have it explicit). So having 
>> some costs/changes upstream for downstream differences like this seems at 
>> least vaguely plausible to me.
>
> We do our executable renaming post build and lit testing. We do the renaming 
> to nearly all our built tools, not just the clang (clang++ etc) family, e.g. 
> the LLVM binutils like llvm-objdump becomes xxx-llvm-objdump, so unless the 
> scope of this increases to include those, I'm not sure how useful it would be 
> to us (and expanding the scope to other tools becomes problematic because 
> there isn't a `%clang` equivalent for testing purposes for those other tools, 
> so presumably would require significantly more updates?).
>
> @probinson may have more thoughts on this though.

No, you've stated my thoughts almost exactly.  If this became a generic 
prefixing thing, we'd want to apply it to clang, llvm tools, lld, etc.

Re the `%clang` equivalent, actually there is already tool-name substitution 
without the % in order to add the build-dir path to the tool name, so I *think* 
that can be made to work without having to update every test in existence.  
But, if the prefix isn't added universally, it's not really useful for us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: jhenderson, probinson, dblaikie.
dblaikie added a comment.

Can't say I'm super enthusiastic about this (I assume the build already 
supports prefixes and suffixes, which I'd hope would be adequate - but 
presumably are not for your use case), though there's some, I think, related 
prior art: Sony folks (@probinson @jhenderson) have (or had at some point) 
different C++ language standard/version defaults than upstream and have 
maintained/made changes to upstream test cases that assume the upstream default 
version to not make that assumption (to have it explicit). So having some 
costs/changes upstream for downstream differences like this seems at least 
vaguely plausible to me.

I guess I might use this if it existed to have shorter names for my development 
build (rather than `clang-tot` maybe I could name it `clot` or something... ).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai created this revision.
nemanjai added reviewers: rjmccall, rsmith, craig.topper, t.p.northover, 
arsenm, kparzysz, echristo.
Herald added subscribers: usaxena95, s.egerton, kadircet, arphaman, delcypher, 
simoncook, mgorny.
nemanjai requested review of this revision.
Herald added subscribers: llvm-commits, Sanitizers, wdng.
Herald added projects: clang, Sanitizers, LLVM, clang-tools-extra.

It is likely that at least some vendors would like the ability to rename the 
clang executable to a proprietary name. That would allow a clear 
differentiation between the upstream clang and a proprietary downstream version 
of clang on the same system.

I am not sure if there are vendors that already do this downstream but it seems 
like a useful capability to allow that to be parameterized in the CMake 
configuration and an upstream buildbot to be added that builds with a different 
name. We at IBM certainly desire this capability and are willing to convert one 
of our upstream buildbots to such a build.

This patch provides a new CMake macro `CLANG_EXE_NAME` that can be used to 
specify the name of the clang executable. Then the symlinks to it (i.e. 
`clang-cl` and `clang++` will use that name and append the appropriate suffix). 
Furthermore, this patch updates test cases and utilities that use `clang` to 
use this name. The name is added to `llvm-config` so that it can be queried 
using `llvm-config --clang-exe` (which will return the full path with the 
correct name of the clang executable).

This is a work in progress because it is incomplete. Depending on the feedback 
on this review, the name change can affect the various `clang-xx` utilities, 
`libclang...` libraries, etc.

Please provide feedback if you think this is useful, not useful, a good/bad 
idea, good/bad technical direction etc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103125

Files:
  clang-tools-extra/clangd/test/lit.site.cfg.py.in
  clang-tools-extra/test/CMakeLists.txt
  clang-tools-extra/test/lit.site.cfg.py.in
  clang/CMakeLists.txt
  clang/include/clang/Config/config.h.cmake
  clang/runtime/CMakeLists.txt
  clang/test/Analysis/check-analyzer-fixit.py
  clang/test/Analysis/scan-build/cxx-name.test
  clang/test/CMakeLists.txt
  clang/test/Driver/baremetal.cpp
  clang/test/Driver/check-time-trace.cpp
  clang/test/lit.cfg.py
  clang/test/lit.site.cfg.py.in
  clang/test/utils/update_cc_test_checks/lit.local.cfg
  clang/tools/clang-offload-bundler/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/driver/CMakeLists.txt
  clang/utils/perf-training/CMakeLists.txt
  compiler-rt/cmake/Modules/AddCompilerRT.cmake
  compiler-rt/cmake/base-config-ix.cmake
  llvm/cmake/modules/LLVMExternalProjectUtils.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/test/lit.site.cfg.py.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/llvm-rc/llvm-rc.cpp
  llvm/utils/lit/lit/llvm/config.py
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -114,6 +114,17 @@
 
 
 def infer_dependent_args(args):
+  if not args.llvm_bin:
+llvm_config = 'llvm-config'
+  else:
+llvm_config = os.path.join(args.llvm_bin, 'llvm-config')
+
+  # If the clang executable has a different name and we knoww where to
+  # get llvm-config, we can get the name that way.
+  if not args.clang and distutils.spawn.find_executable(llvm_config):
+args.clang = subprocess.check_output(
+  [llvm_config, '--clang-exe']).decode().strip()
+
   if not args.clang:
 if not args.llvm_bin:
   args.clang = 'clang'
Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -502,7 +502,7 @@
 
 # Discover the 'clang' and 'clangcc' to use.
 self.config.clang = self.use_llvm_tool(
-'clang', search_env='CLANG', required=required,
+self.config.clang_exe_name, search_env='CLANG', required=required,
 use_installed=use_installed)
 if self.config.clang:
   self.config.available_features.add('clang')
Index: llvm/tools/llvm-rc/llvm-rc.cpp
===
--- llvm/tools/llvm-rc/llvm-rc.cpp
+++ llvm/tools/llvm-rc/llvm-rc.cpp
@@ -36,6 +36,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Config/config.h"
 
 #include 
 #include 
@@ -130,14 +131,14 @@
   if (!Parent.empty()) {
 // First look for the tool with all potential names in the specific
 // directory of Argv0, if known
-for (const auto *Name : {"clang", "clang-cl"}) {
+for (const auto *Name : {"clang", "clang-cl", CLANG_EXE_NAME}) {
   Path = 

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D103125#2780936 , @dblaikie wrote:

> Can't say I'm super enthusiastic about this (I assume the build already 
> supports prefixes and suffixes, which I'd hope would be adequate - but 
> presumably are not for your use case), though there's some, I think, related 
> prior art: Sony folks (@probinson @jhenderson) have (or had at some point) 
> different C++ language standard/version defaults than upstream and have 
> maintained/made changes to upstream test cases that assume the upstream 
> default version to not make that assumption (to have it explicit). So having 
> some costs/changes upstream for downstream differences like this seems at 
> least vaguely plausible to me.

We do our executable renaming post build and lit testing. We do the renaming to 
nearly all our built tools, not just the clang (clang++ etc) family, e.g. the 
LLVM binutils like llvm-objdump becomes xxx-llvm-objdump, so unless the scope 
of this increases to include those, I'm not sure how useful it would be to us 
(and expanding the scope to other tools becomes problematic because there isn't 
a `%clang` equivalent for testing purposes for those other tools, so presumably 
would require significantly more updates?).

@probinson may have more thoughts on this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103125

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits