Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be fast by default #630

Open
ob opened this issue Dec 20, 2022 · 10 comments
Open

Be fast by default #630

ob opened this issue Dec 20, 2022 · 10 comments

Comments

@ob
Copy link
Member

ob commented Dec 20, 2022

Most people with moderate to large apps ends up configuring a set of options to make it fast:

build --features swift.cacheable_swiftmodules
build --features swift.use_global_module_cache
build --features apple.virtualize_frameworks
build --features apple.arm64_simulator_use_device_deps

build --features=swift.opt_uses_wmo
build --swiftcopt=-whole-module-optimization

We should turn these options on by default so that people don't need to figure out how to make it fast or worse, put up with slow builds 🐢 ...

@mattrobmattrob
Copy link
Collaborator

FWIW, @brentleyjones is likely motivated to do the same for https://github.com/bazelbuild/bazel + rules_apple + rules_swift.

@brentleyjones
Copy link

Yeah, I think the rules_swift ones we should flip at the rules level as well. I've added it to my todo list. The only one we wouldn't apply by default WMO, as that one is more subjective. Btw, swift.opt_uses_wmo is enabled by default: https://github.com/bazelbuild/rules_swift/blob/1c1d7a89f0d63860abacf2fb373e50a43b625453/swift/internal/swift_toolchain.bzl#L287

@jerrymarino
Copy link
Contributor

I think the tradeoff is 1-2 of these might produce errors that don't have a solution. The upside is getting more people to report bugs or fix bugs for the happy path. As long as it's possible / easy to flip them off it SGTM.

PS: I'd suggest fixing this one will means ripping out the code to make it conditional

build --features apple.arm64_simulator_use_device_deps

AFAIK there should be no downside. I seen bug reports / pings about it being broken without this.

@brentleyjones
Copy link

Isn't a potential downside of apple.arm64_simulator_use_device_deps that a device dep can have something like #if targetEnvironment(simulator) and then work incorrectly on simulator?

@jerrymarino
Copy link
Contributor

and then work incorrectly on simulator?

There isn't a general answer I have as it requires consideration and knowledge of the application and usage to optimize / enable something like this. Same applies for other optimizations here and ones coming soon 😉

@acecilia
Copy link
Contributor

acecilia commented Dec 20, 2022

What about grouping all these tweaks that are too unsafe to turn ON by default under a single feature flag (or alike), so:

  • There is no need to officially turn them ON by default in the rules
  • Still, users can easily turn them ON without having to figure out how to do it or their details
  • There is a single source of truth for them

For example: build --features apple.fastest_configuration

@brentleyjones
Copy link

brentleyjones commented Dec 21, 2022

@jerrymarino sure, but you said "AFAIK there should be no downside", and I was giving you a downside, and why you should allow it it stay conditional.

@jerrymarino
Copy link
Contributor

jerrymarino commented Dec 21, 2022

@brentleyjones thanks for the suggestion 👍

Given the goal of the feature is to enable M1 builds that otherwise wouldn't work. In other words, if you aren't able to use the feature - is there a reason you can't just use the x86 in that case?

If you are relying on the errors it prints to not use the M1 slice - we can improve the developer messaging instead:

Building this app on M1 simulator isn't supported

Anecdotally I have gotten a lot of feedback on how this enabled people to use Apple silicon 📈 . The majority of SDKs people believed they couldn't ran on a simulator turned out to be fine because the developers never used them on a simulator anyways

@jerrymarino
Copy link
Contributor

jerrymarino commented Dec 21, 2022

Also I'm pretty sure there is a bug in cocoapods-bazel with that disabled:
bazel-ios/cocoapods-bazel#84

@brentleyjones
Copy link

brentleyjones commented Dec 21, 2022

bazelbuild/rules_swift@c7ed06e
bazelbuild/rules_swift@cb64a41

We will cut a new rules_swift release today as well.

Edit: released https://github.com/bazelbuild/rules_swift/releases/tag/1.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants