sepavloff accepted this revision.
sepavloff added a comment.
This revision is now accepted and ready to land.

In D66834#1685921 <https://reviews.llvm.org/D66834#1685921>, @broadwaylamb 
wrote:

> In D66834#1685260 <https://reviews.llvm.org/D66834#1685260>, @sepavloff wrote:
>
> > In D66834#1653334 <https://reviews.llvm.org/D66834#1653334>, @broadwaylamb 
> > wrote:
> >
> > > In D66834#1652756 <https://reviews.llvm.org/D66834#1652756>, @compnerd 
> > > wrote:
> > >
> > > > I think that this is pretty easy to forget.  Fortunately, last argument 
> > > > wins.  Why not sink this into the `%clang` substitution in lit?  That 
> > > > ensures that we run with an empty sysroot and then when the test needs 
> > > > to adjust the sysroot, it can do so explicitly.
> > >
> > >
> > > I've just tried to do it, but unfortunately some tests are failing, for 
> > > example, `Driver/cc1-response-files.c`. The problem is that `%clang` is 
> > > expanded to `/path/to/clang --sysroot=`, but the succeeding flags (such 
> > > as `-cc1`) may be incompatible with `--sysroot`.
> >
> >
> > Does the issue manifests itself with `-cc1` only? We usually use 
> > `%clang_cc1` in such cases, so probably those tests require update.
>
>
> Some Driver tests take a list of arguments from a file (for example 
> `clang/test/Driver/cc1-response-files.c`), so we probably cannot use 
> `%clang_cc1` there


The obvious solution in this case is to implement support of an environment 
variable, say `SYSROOT` or `CLANG_SYSROOT`, which would override hard-coded 
path (but not that specified by --sysroot option). Unfortunately some 
developers don't like use of environment variables, as it creates 'secret' 
control channel, and such patch would have high chance to be rejected. So 
probably this is all we can do here.

This patch looks good for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66834



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

Reply via email to