Skip to content

Conversation

ob
Copy link
Member

@ob ob commented May 19, 2022

Since this performs better and we keep telling people to enable it, maybe we should make it the default?

Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is now pretty robust and stabilized and ready for prime. Most of the big builds I've heard about using rules_ios had to enable it because the perf was bad otherwise. Atop from build perf, the IDE is improved with this from multiple angles!

Do you have a strong reason to rip it out: e.g. it's hard to maintain or broken for the other case? Otherwise, I'd suggest to add an opt in / add opt out flag --features apple.incompatible_disable_virtual_frameworks and give it ~6-12 months to bake. This will have the effect of migrating people and giving ones we break reasonable to fix their issues or doing something else it if they don't / can't want to use it like this.

@jerrymarino
Copy link
Contributor

We can also remove the dup'd over github action shards when it's gone!

@ob
Copy link
Member Author

ob commented May 21, 2022

Do you have a strong reason to rip it out: e.g. it's hard to maintain or broken for the other case? Otherwise, I'd suggest to add an opt in / add opt out flag --features apple.incompatible_disable_virtual_frameworks and give it ~6-12 months to bake. This will have the effect of migrating people and giving ones we break reasonable to fix their issues or doing something else it if they don't / can't want to use it like this.

Yeah, this is a better plan.

@luispadron
Copy link
Collaborator

There might be some tests to add / use cases to fix before we enable this as default. For example: #500 does not work with VFS enabled. Im not sure if this is a one-off issue or if we'll see more issues like this by flipping it on.

@luispadron
Copy link
Collaborator

#500 is now closed so making this the default / cleaning up where the flags are used seems like a reasonable change.

@jerrymarino
Copy link
Contributor

I'm still hoping to make it default but leave it in - unless it becomes a big maintenance problem for us or other reasons to really gut it! If you've got to another use case e.g. #500 we should try to triage and resolve them. Making it default also might encourage people with issues to fix it but atleast give the escape hatch

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

Successfully merging this pull request may close these issues.

3 participants