Implement 'first pass' of index while building V2 #217
Conversation
| index_while_building_objc_copts = select({ | ||
| "@build_bazel_rules_ios//:index_while_building_v2": [ | ||
| # Note: this won't work work for remote caching yet. It uses a | ||
| # _different_ global index for objc than so that the BEP grep in |
There was a problem hiding this comment.
Needs more work for clang - I've outlined the entire solution to index while building V2 here: https://github.com/bazel-ios/rules_ios/blob/master/docs/index_while_building.md#objc-remote-cache-support
| @@ -53,13 +53,15 @@ def rules_ios_dependencies(): | |||
| sha256 = "1fff3fa1e565111a8f678b4698792101844f57b2e78c5e374431d0ebe97f6b6c", | |||
| ) | |||
|
|
|||
| # Note: this is a branch of the rules_swift PR | |||
| # https://github.com/bazelbuild/rules_swift/pull/567 | |||
There was a problem hiding this comment.
Do you plan to merge this PR after merging the rules_swift PR?
There was a problem hiding this comment.
@congt we ended up deciding to to use a short lived fork of these dependencies to let this bake and reap the benefits while upstream refactoring is worked out.
There was a problem hiding this comment.
I'm so sorry, I think there was some miscommunication in that meeting on Wednesday. I talked with @segiddins and @amberdixon and we'd heard different things (Sam closer to me, Amber closer to you), so my apologies for not ensuring we were all on the same page about next steps.
I'm super excited to get this landed and the immediate impact it will have. Is there an impediment to upstreaming the changes, or is it the work to address PR feedback and gathering consensus on the approach? Are we concerned there will be pushback overall?
In general, we've taken the approach of attempting to upstream changes first, and only resorting to forking if we can't get the changes landed. If we hit a wall upstreaming the changes, then we should reevaluate. My perspective is that it is harder to unfork things as time goes on, and there isn't as much incentive to unfork, which is more likely to lead to a fork becoming long lived (which has associated maintenance cost). I'm aware that this means it takes longer to realize the impact of the work. I'm definitely open to other perspectives here, both from you and the rest of the team.
There was a problem hiding this comment.
I agree to getting this upstreamed and want to get it working for all Bazel users. We should try to work with the maintainers of rules_swift to get it landed. That being said, the PR has been open at rules_swift since last Friday which should be upstream able. I also mentioned the short lived fork in our plan to move this forward https://github.com/bazel-ios/rules_ios/pull/212/files#diff-26cd539e3b611f02c1703c8fce4656a533b33dc08a4479ab5bfa7523260fa292R123
@segiddins can you list the required changes to land this? There's been a lot of ideas shared and back and forth on the PR
There was a problem hiding this comment.
@justinseanmartin Here is what I propose for the next steps:
- keep the temp fork of rules_swift/index-import until everyone feels good about landing the PR ( and weeks of testing internally )
- land the PR in rules_ios that points at the forks of index-import and rules_swift
- point repos at HEAD of rules_ios after the PR lands
Overall we might want to have a larger discussion with the community around landing PRs and features into rules_swift behind feature flags. It's a great way to share ideas. Not everyone has access to work that way ATM.
There was a problem hiding this comment.
Overall, we're in agreement with the commitment to actively work to get unforked in the near term (weeks not months), taking an active role in reaching out and collaborating with rules_swift and index-import maintainers.
There was a problem hiding this comment.
@justinseanmartin SGTM - I'm fully in support to getting this un-forked in some form 👍
Longer term I hope we can get a flag to handle this in swift / clang: that'd be the ideal state for this specific issue. The main reason we need to the helper binary ( e.g. my PR to index-import ) in rules_swift is that we can't tell the compiler to index only impl files. We'll also need to wait for that release cycle if we go with the swift/clang approach, that is if we could get it landed in there.
jerrymarino
force-pushed
the
jmarino/index_while_building_v2_first_pass
branch
from
762ab1d to
ec6a67f
Compare
This concludes the first step if mitigating perf issues and first PR in the series for index while building - docs/index_while_building.md. Hinging on the feature, swift.index_while_building_v2, it moves rules_ios to use a global index and pulls in the swift PR bazelbuild/rules_swift#567, and index-import PR MobileNativeFoundation/index-import#53 . In hopes of mitigating performance problems of processing the global index in this way it flips the `-incremental` bit in the latest flag. Longer term we will not be processing a global index, per docs/index_while_building.md so this is temporary measure. This PR implements line items from 'Index while building V2 - first pass' in the roadmap for this feature defined in docs/index_while_building.md.
jerrymarino
force-pushed
the
jmarino/index_while_building_v2_first_pass
branch
from
92e164f to
aedaf35
Compare
|
Seems good for now, for a followup we might want to consider moving the |
* Implement 'first pass' of index while building V2 This concludes the first step if mitigating perf issues and first PR in the series for index while building - docs/index_while_building.md. Hinging on the feature, swift.index_while_building_v2, it moves rules_ios to use a global index and pulls in the swift PR bazelbuild/rules_swift#567, and index-import PR MobileNativeFoundation/index-import#53 . In hopes of mitigating performance problems of processing the global index in this way it flips the `-incremental` bit in the latest flag. Longer term we will not be processing a global index, per docs/index_while_building.md so this is temporary measure. This PR implements line items from 'Index while building V2 - first pass' in the roadmap for this feature defined in docs/index_while_building.md.
This concludes the first step of mitigating perf issues and first PR in
the series for index while building - docs/index_while_building.md.
Hinging on the feature, swift.index_while_building_v2, it moves
rules_ios to use a global index and pulls in the swift PR
bazelbuild/rules_swift#567, and index-import PR
MobileNativeFoundation/index-import#53 .
In hopes of mitigating performance problems of processing the global
index in this way it flips the
-incrementalbit.Longer term we will not be processing a global index, per
docs/index_while_building.md so this is temporary measure.
This PR implements line items from 'Index while building V2 - first
pass' in the roadmap for this feature defined in
docs/index_while_building.md.