This is an archive of the discontinued LLVM Phabricator instance.

[lit] Parse command-line options from LIT_OPTS
ClosedPublic

Authored by jdenny on Jul 3 2019, 7:59 AM.

Details

Summary

Similar to FILECHECK_OPTS for FileCheck, LIT_OPTS makes it easy to
adjust lit behavior when running the test suite via ninja. For
example:

$ LIT_OPTS='--time-tests -vv --filter=threadprivate' \
  ninja check-clang-openmp

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny created this revision.Jul 3 2019, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 7:59 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
rnk added a comment.Jul 3 2019, 1:42 PM

I think it would be preferable to standardize on llvm-lit as the way to re-run particular tests with some options.

rnk removed a reviewer: rnk.Jul 3 2019, 1:42 PM
jdenny added a subscriber: rnk.Jul 3 2019, 2:04 PM
In D64135#1569174, @rnk wrote:

I think it would be preferable to standardize on llvm-lit as the way to re-run particular tests with some options.

Thanks for the comment. Why so?

For me, it seems quicker to use LIT_OPTS and ninja because I don't have to extract the lit command line from ninja -v. For ninja check-all, that's a lot to copy and paste. It also avoids forgetting to rebuild while I'm alternating between hacking the source and re-running failing tests.

jdenny removed a subscriber: rnk.Jul 3 2019, 2:25 PM

lit has options?

If this works when running llvm-lit.py as well as ninja check-foo, I can see it being handy for those (rare at least for me) occasions when a lit option is what you want. And it looks pretty cheap.

Re tests, I don't see why you have 4, wouldn't 2 be sufficient? Given that 2, 3, and 4 are identical.

jdenny added a comment.Jul 3 2019, 3:45 PM

lit has options?

:-)

If this works when running llvm-lit.py as well as ninja check-foo, I can see it being handy for those (rare at least for me) occasions when a lit option is what you want. And it looks pretty cheap.

Lately, I most often use -vv (so I can see the line number of a failed RUN command) and -a (so I can verify the passing tests are doing what I expect).

Re tests, I don't see why you have 4, wouldn't 2 be sufficient? Given that 2, 3, and 4 are identical.

Good point. Actually, 1 is probably enough for what I actually mean to test. I'll adjust soon.

Thanks.

jdenny updated this revision to Diff 208055.Jul 4 2019, 10:10 AM

Simplify test as discussed with @probinson. Improve wording in docs slightly.

This revision is now accepted and ready to land.Jul 7 2019, 3:37 PM
This revision was automatically updated to reflect the committed changes.
yln added a subscriber: yln.Oct 10 2019, 1:19 PM

Hi Joel @jdenny,
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 understand the desire for this so one can reinvoke env LIT_OPTS=... ninja check-... and be certain that those options are going to be used. My arguments against it are mostly for consistency and to avoid surprising non-standard behavior:

  • It is not "standard". The default is that CL options override ENV vars [1].
  • It is inconsistent with the other option configurable var env vars, e.g., LIT_FILTER and LIT_*_SHARD.
  • Most of the time we want to supplement---not override---existing options.

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?

env_args = shlex.split(os.environ.get("LIT_OPTS", ""))
args = sys.argv[1:] + env_args  # lit-opts.py does not go red when we flip the order of concatenation here. Is this expected?
opts = parser.parse_args(args)

Also: thank you for adding and documenting this feature and even providing a test. I am already using it and find it useful!

[1] https://stackoverflow.com/a/11077282/271968

In 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).
  1. 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.