[PATCH] D64135: [lit] Parse command-line options from LIT_OPTS
Joel E. Denny via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 10 17:13:17 PDT 2019
jdenny added a comment.
In D64135#1704685 <https://reviews.llvm.org/D64135#1704685>, @yln wrote:
> Hi Joel @jdenny,
Hi Julian.
> I would like to ask you to reconsider whether or not it is a good idea that the env var overrides the command line options.
I guess it depends on the intended use case: which is the default, and which is the override?
1. I added `LIT_OPTS` specifically in order to override the default options supplied by the build config. That implies `LIT_OPTS` should be parsed last (assuming later options override earlier options when order matters).
2. I think the convention you mention is for the (probably much more common) use case where an environment variable is intended to hold a default that can be overridden on the command line. Do we anticipate that people will actually use `LIT_OPTS` for that purpose?
To fully support the use case 1 without risking surprising behavior if someone attempts use case 2, we could try to move the `LIT_OPTS` implementation out of lit and into the build system. That is, use case 2 would become impossible.
Then again, order doesn't seem to matter anyway for options that I normally care about: `-a` and `-vv` seem to take effect no matter where they appear in relation to the usual default of `-sv`. Order does matter for `--filter`, and I assume it matters for `-D`. I don't know if anyone would ever build with those as default options and then want to override them. It might be surprising if you couldn't, given that 1 is the intended use case.
Without knowing which use cases beyond my own are real, I'm inclined to wait before selecting a path forward. @probinson reviewed, so perhaps he has some opinion.
> One more ask independent of what we decide: the `lit-opts.py` test does not go red when we change the override order. Can you adapt the test to give a signal in this case?
Good point.
> Also: thank you for adding and documenting this feature and even providing a test. I am already using it and find it useful!
Good to hear!
Thanks.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64135/new/
https://reviews.llvm.org/D64135
More information about the llvm-commits
mailing list