Page MenuHomePhabricator

[Modules] Add platform feature to requires clause
ClosedPublic

Authored by bruno on Sep 10 2018, 9:47 PM.

Details

Summary

Allows module map writers to add build requirements based on platform/os. Useful when target features and language dialects aren't enough to conditionalize building a module. Among other things, it also makes it more convenient for modules with the same name but different set of submodules to be described in the same module map.

rdar://problem/43909745

Diff Detail

Repository
rL LLVM

Event Timeline

bruno created this revision.Sep 10 2018, 9:47 PM
aprantl added inline comments.Sep 11 2018, 8:22 AM
docs/Modules.rst
470 ↗(On Diff #164802)

Does this work with platforms+environment combinations, such as the ios simulator on Darwin?

bruno updated this revision to Diff 165612.Sep 14 2018, 4:33 PM

Addressed Adrian's review. Added support to consider the environment and well the combination platform-environment. @aprantl also handled multiple variants of simulator combinations.

aprantl added inline comments.Sep 14 2018, 4:47 PM
docs/Modules.rst
470 ↗(On Diff #164802)

Is iossimulator a thing? The triple is called x86_64-apple-ios-11.2.3-simulator and the SDK calls itself iphonesimulator, so I wonder if this shouldn't be called ios-simulator or iphonesimulator instead.

aprantl added inline comments.Sep 14 2018, 4:51 PM
lib/Basic/Module.cpp
101 ↗(On Diff #165612)

Ah.. it looks like you are fully aware of this :-)

rsmith added inline comments.Sep 14 2018, 4:52 PM
docs/Modules.rst
476–477 ↗(On Diff #165612)

What is the reason to allow these to be combined into a single feature name rather than treating them as two distinct features? (Eg, requires linux, gnueabi rather than requires linux-gnueabi)?

bruno added inline comments.Sep 17 2018, 2:42 PM
docs/Modules.rst
476–477 ↗(On Diff #165612)

No specific reason other than the convenience of writing triple like requirements. But thinking more about it, it might make more sense to have it decoupled and written as a combination to avoid confusion. Will update the patch to reflect that.

bruno updated this revision to Diff 165861.Sep 17 2018, 6:22 PM

Update patch after review.

rsmith accepted this revision.Sep 17 2018, 8:25 PM
rsmith added inline comments.
lib/Basic/Module.cpp
81 ↗(On Diff #165861)

Comment is out of date.

This revision is now accepted and ready to land.Sep 17 2018, 8:25 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.