This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Always include custom syslibroot when running tests
ClosedPublic

Authored by int3 on Sep 18 2020, 10:05 PM.

Details

Summary

This greatly reduces the amount of boilerplate in our tests.

Diff Detail

Event Timeline

int3 created this revision.Sep 18 2020, 10:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 10:05 PM
int3 requested review of this revision.Sep 18 2020, 10:05 PM
int3 updated this revision to Diff 292941.Sep 18 2020, 10:15 PM

update search-paths.test

int3 edited the summary of this revision. (Show Details)Sep 18 2020, 10:18 PM
smeenai requested changes to this revision.Sep 22 2020, 11:58 AM
smeenai added subscribers: MaskRay, smeenai.

The test cleanups are really nice, but I'm not a fan of coding this sort of test-specific behavior (especially the path) into LLD itself :/ (I know we have some precedent with LLD_IN_TEST, but that's only used in canExitEarly and it's a pretty minor behavior change.)

Given the previous objection to substitutions though, this might be the best solution. I think I'd feel better about it if the environment variable just contained the syslibroot path directly, because then the path (which is test-specific knowledge) is fully contained in the test configs.

Also adding @MaskRay in case he has any thoughts on substitutions vs. environment variables.

(Requesting changes to put back in your queue.)

This revision now requires changes to proceed.Sep 22 2020, 11:58 AM
int3 updated this revision to Diff 293760.Sep 23 2020, 8:53 AM

put entire mach-o syslibroot path in the environment variable

compnerd requested changes to this revision.Sep 23 2020, 2:03 PM
compnerd added a subscriber: compnerd.

Emphatic +1 to @smeenai's comment. This really should be done via lit substitutions.

This revision now requires changes to proceed.Sep 23 2020, 2:03 PM
int3 added a comment.Sep 23 2020, 2:05 PM

One downside of lit substitutions is that running llvm-lit <test> will then generate a temporary file in the current directory.

But I don't really care one way or another -- I did it this way primarily because of @ruiu's preference.

int3 updated this revision to Diff 294193.Sep 24 2020, 3:42 PM

use substitution

int3 edited the summary of this revision. (Show Details)Sep 24 2020, 3:43 PM
int3 edited the summary of this revision. (Show Details)
compnerd accepted this revision.Sep 24 2020, 3:44 PM

lld changes seem unrelated (include changes, whitespace); LGTM with the copyright header for lit.cfg

lld/test/MachO/lit.local.cfg
3

Please add the copyright header

int3 added inline comments.Sep 24 2020, 3:51 PM
lld/test/MachO/lit.local.cfg
3

hmm I looked at a bunch of other lit.cfg.py files and they all seemed to lack the copyright header

This revision was not accepted when it landed; it landed in state Needs Review.Sep 25 2020, 11:29 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.