Page MenuHomePhabricator

[libc++] Provide 'buildhost-<platform> feature for the tests.
ClosedPublic

Authored by ldionne on May 6 2021, 9:32 PM.

Details

Summary

The target platform could differ from the host platform for the cross platform builds. Some tests are depended on the build host features and they need to determine a proper platform environment.

This commit adds a build host platform name feature for the libc++ tests in format buildhost-<platform>, such as buildhost-linux, buildhost-darwin, buildhost-windows & etc.

The Windows host gets two features: one buildhost-windows and another based on Windows "sub-system", such as buildhost-win32, buildhost-cygwin & etc.

Diff Detail

Event Timeline

vvereschaka created this revision.May 6 2021, 9:32 PM
vvereschaka requested review of this revision.May 6 2021, 9:32 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 6 2021, 9:32 PM

Thanks for the patch. I'd like to sit on this for a bit and implement a more general mechanism for detecting the host triple vs the target triple. This is related to my work in D102012.

got it, perfect. Thank you.

ldionne requested changes to this revision.Jul 5 2021, 12:40 PM

Now that D104747 has landed, we can look at this again.

I suggest we add a feature named host=<name-of-the-host>. Would it make sense for that to be a full triple instead of just the result of sys.platform?

This revision now requires changes to proceed.Jul 5 2021, 12:40 PM

Also, where would that feature be used? It would be helpful for me to understand which tests benefit from that feature. In general, we try not to add features if they are not used anywhere.

Also, where would that feature be used? It would be helpful for me to understand which tests benefit from that feature. In general, we try not to add features if they are not used anywhere.

I don't think it matters to have a full triple for the build host - in a cross testing scenario, we know the exact triple of the target we're compiling for, but we know pretty little about the host we're driving the tests from - within the python environment we don't know much else than what python tells us.

Also, where would that feature be used? It would be helpful for me to understand which tests benefit from that feature. In general, we try not to add features if they are not used anywhere.

I think this was meant to be used as a different workaround for the thing that was fixed in D102048, but there might be other cases that could need this as well.

Would it make sense for that to be a full triple instead of just the result of sys.platform?

I think it would be much easier using а short representation of platform name instead of a full triple and especially if it corresponds to the target platform names, but with the prefixes or something similar.
Also it is not always possible to get a correct triple for the windows platform.

Also, where would that feature be used?

Not sure it will be used often.

I suggest we add a feature named host=<name-of-the-host>.

No objection, but looks like unusual representation for the feature.
So it may look like that:

// REQUIRES: host=linux && host=darwin
...
// UNSUPPORTED: host=windows

is this that expected?

ldionne accepted this revision.Jul 28 2021, 9:19 AM

I suggest we add a feature named host=<name-of-the-host>.

No objection, but looks like unusual representation for the feature.
So it may look like that:

// REQUIRES: host=linux && host=darwin
...
// UNSUPPORTED: host=windows

is this that expected?

Yes. Lit supports = in features. We don't use it often, but it's definitely valid, and I think it makes sense especially since we have target=<triple> now. Note that the same argument could be made for target=<triple>, but I think it's better than target-<triple> because there's already dashes in the triple, and not saying target anywhere makes it ambiguous.

Ack regarding the fact that a full triple for the host is overkill, I'm convinced now.

LGTM with my suggested edits.

libcxx/utils/libcxx/test/features.py
156–159
This revision is now accepted and ready to land.Jul 28 2021, 9:19 AM
ldionne commandeered this revision.Fri, Sep 3, 12:27 PM
ldionne edited reviewers, added: vvereschaka; removed: ldionne.
This revision now requires review to proceed.Fri, Sep 3, 12:27 PM
ldionne updated this revision to Diff 370646.Fri, Sep 3, 12:28 PM
ldionne edited the summary of this revision. (Show Details)

Apply comments.

Will ship this once the CI is green. Thanks for the patch @vvereschaka !

ldionne accepted this revision.Tue, Sep 7, 8:49 AM

CI is green except for the runtimes build, which is a failure that was fixed on main.

This revision is now accepted and ready to land.Tue, Sep 7, 8:49 AM
This revision was landed with ongoing or failed builds.Tue, Sep 7, 8:49 AM
This revision was automatically updated to reflect the committed changes.