Page MenuHomePhabricator

[gn build] mac: use frameworks instead of libs where appropriate
ClosedPublic

Authored by markmentovai on Jul 20 2020, 8:30 PM.

Details

Summary

As of GN 3028c6a426a4, the hack that transformed "libs" ending in
".framework" from -l arguments to -framework arguments has been removed.
Instead, "frameworks" must be used, and the toolchain must provide
support.

Diff Detail

Event Timeline

markmentovai created this revision.Jul 20 2020, 8:30 PM
Herald added a project: Restricted Project. · View Herald Transcript

(full context)

Thanks for the patch!

This will break the build with old GNs, right? Should we make this dependent on gn_version to keep both versions working for a while? (As-is, it'll break my mac bot.)

markmentovai added a comment.EditedJul 21 2020, 1:55 PM

Thanks for the patch!

This will break the build with old GNs, right? Should we make this dependent on gn_version to keep both versions working for a while? (As-is, it'll break my mac bot.)

It’d have to be a six-month-old GN or older, which seems like a long enough window. GN has had support for “frameworks” since https://gn.googlesource.com/gn/+/a09ec16183b550073c04fa8eeee230047835d118, 2020-01-16.

If you think you need a wider window than that for the LLVM GN build, I’ll accommodate with a gn_version check (and, note to self, the magic number is ≥ 1693). But if your bot’s GN has been upgraded in the past six months and you agree that six months is older than how young we’d expect people’s GNs to be, the gn_version check shouldn’t be necessary.

If you just want a better message than a mysterious failure to help users out, maybe we could carry this for a little while?

assert(gn_version >= 1693, "Update GN, perhaps by running llvm/utils/gn/get.py")

Nico, ping. What do you think about the findings and the options?

Fair enough, an assert seems ok then.

I walked over to my bot and it's currently using gn 1622 from Aug 2019, but if there's a friendly assert that tells me that I need to update and how to do that, I can probably figure out how to do that :)

Add assertion as discussed

thakis accepted this revision.Aug 6 2020, 3:40 PM

Thanks!

This revision is now accepted and ready to land.Aug 6 2020, 3:40 PM
This revision was landed with ongoing or failed builds.Aug 6 2020, 3:59 PM
This revision was automatically updated to reflect the committed changes.
yannic added a subscriber: yannic.Aug 7 2020, 2:31 AM

I ran into the same issue a few days ago.
Thanks for fixing this!