Add Generated Files with Package

[sebastianv1]

Related issue: https://github.com/MobileNativeFoundation/rules_xcodeproj/issues/3037

Colocates generated files with its owning package in the Xcode project. Finding and inspecting a module's generated files can be quite difficult today with only the top-level Bazel Generated Files group when working on a specific library or module.

Some initial questions I have from this v1 implementation:

• Is -- a fine delimiter for passing along the owner package for each File?
• I'd like to reuse the existing create group callables but noticed we use a stable ID for the special top-level groups. Any weird side effects I might be missing by generating IDs similar to other groups for nested generated files (keeping the same stable ID for the top level special groups)
• One potential edge case is if an owner's group doesn't exist in the project, in theory this will still create the owning group even if there are no source files in the generated project. I imagine this is probably more of an issue with focused projects? I may need to add some sort of preprocessor or filtering to do this check.
• It looks like external repos don't have an owner. I think this would still be a helpful feature, we'd just associate any generated files based on the path with the first directory under external if that seems reasonable?

Testing from Integration example

ration.xcodeproj

[brentleyjones]

@erikkerber Can you take an initial review, since you've requested this feature in the past?

[erikkerber]

Nice! Let me check it out

[erikkerber]

Seeing this locally and on CI. Were there some un-pushed changes?

error: 'nil' requires a contextual type
                sourceTree: nil

[sebastianv1]

@erikkerber Forgot to include a local change. Just updated and should work.

[erikkerber]

Without looking at the impl yet, this is exciting so far.

One quirky edge case we're seeing in our project is that generated files for an external repository are showing up at the top-level of our project (this repo is an iOS SDK we develop internally that is pulled in via a repo rule). We don't want that for sure, but I'm also not sure what the approach is to omit those. Maybe the same general problem as:

One potential edge case is if an owner's group doesn't exist in the project, in theory this will still create the owning group even if there are no source files in the generated project. I imagine this is probably more of an issue with focused projects? I may need to add some sort of preprocessor or filtering to do this check.

[sebastianv1]

@erikkerber Yeah i suspect that might be what I was thinking of. Do you have a simple repro example you could share?

[CognitiveDisson]

Hi. I'd like to share some thoughts based on my testing of this feature.

At a high level, I haven't found any issues with include_package_generated_files_group turned on or off.

However, we're unsure if enabling include_package_generated_files_group is truly beneficial for us. This is due to the various transitions and nested directories in "Bazel Generated Files". If the files could appear without nested directories, near non-generated sources, that would be amazing. But I suppose that's wishful thinking. Given the current structure, it might be convenient in some cases, but in most situations, the search bar is the best tool for navigation.

Another observation is that with the include_package_generated_files_group enabled, we have two files for each generated file in the project. One is in the library folder, and the other is in the top level "Bazel Generated Files". This is as expected from my understanding, but it could be confusing for developers.

Also, it may also increase the project file size, potentially affecting Xcode's overall responsiveness. Of course, the size difference depends on the number of generated files. In our case, the difference was marginal, at just 3%.

This is a great addition, thanks. We might enable the include_package_generated_files_group for our project in the future.

[brentleyjones]

Another observation is that with the include_package_generated_files_group enabled, we have two files for each generated file in the project. One is in the library folder, and the other is in the top level "Bazel Generated Files". This is as expected from my understanding, but it could be confusing for developers.

I wouldn't expect that. When the feature is enabled, they should only exist next to the source files.

[sebastianv1]

@CognitiveDisson Thanks for the feedback and testing!

This is due to the various transitions and nested directories in "Bazel Generated Files". If the files could appear without nested directories, near non-generated sources, that would be amazing

Yeah agreed this can be confusing from the current structure but is a logical part of the build graph considering the different configurations produce different generated files. One potential idea I was debating was providing a configuration rename map (i.e ios_arm64-dbg-ios-arm64-min15 -> iOS 15 Simulator) to give some sense of understanding.

Another observation is that with the include_package_generated_files_group enabled, we have two files for each generated file in the project. One is in the library folder, and the other is in the top level "Bazel Generated Files". This is as expected from my understanding, but it could be confusing for developers.

Currently this is how it works by design, but I can update this design to exclude them from the top-level group. I could see how it might be confusing to see 2 files from the navigator.

[CognitiveDisson]

I'm unsure why this problem didn't occur previously, but with 'include_package_generated_files_group' enabled, I'm now observing numerous top-level groups for 'swift_libraries' with swift protobuf sources generated from proto files. Even after building the project and generating corresponding sources, all groups are marked as missing (shown in "red"). I have applied the latest commits, but the issue persists.

We definitely would not like to see those goups on top level, they generated by an external repository.

Yeah agreed this can be confusing from the current structure but is a logical part of the build graph considering the different configurations produce different generated files. One potential idea I was debating was providing a configuration rename map (i.e ios_arm64-dbg-ios-arm64-min15 -> iOS 15 Simulator) to give some sense of understanding.

Currently I see this structure:

Project
├── ALibraryRoot
│   ├── ALibrary
│   │   ├── Bazel Generated Files
│   │   │   ├── ios_arm64-dbg-ios-arm64-min15
│   │   │   │   └── bin
│   │   │   │       └── ALibraryRoot
│   │   │   │           └── ALibrary
│   │   │   │               ├── subfolder
│   │   │   │                   └── generated.swift
│   │   │   │               └── generated.swift

It would be beneficial if it could consider the build graph and eliminate the redundant structure, like this:

Project
├── ALibraryRoot
│   ├── ALibrary
│   │   ├── Bazel Generated Files
│   │   │   ├── ios_arm64-dbg-ios-arm64-min15
│   │   │   │         ├── subfolder
│   │   │   │             └── generated.swift
│   │   │   │         └── generated.swift

Currently this is how it works by design, but I can update this design to exclude them from the top-level group. I could see how it might be confusing to see 2 files from the navigator.

I don't have a strong opinion on this. Probably better to exclude them from the top-level group.

[sebastianv1]

I'm now observing numerous top-level groups for 'swift_libraries' with swift protobuf sources generated from proto files. Even after building the project and generating corresponding sources, all groups are marked as missing (shown in "red"). I have applied the latest commits, but the issue persists.

Is this the same issue @erikkerber posted above? Do you have an example I can use to repro 🙏 ?

It would be beneficial if it could consider the build graph and eliminate the redundant structure, like this:

I agree that would be a more ergonomic structure. A change like that from what I can tell is much more involved and would require modifying PathTreeNode to pass along a "true" path variable we would set on each modified group tree. Potentially a future improvement to make. Thoughts @brentleyjones ?

[brentleyjones]

I was expecting the second structure as well.

[sebastianv1]

Right, is that blocking this improvement in its current shape?

My concern is from what I can see that looks like a bigger refactor of PathTreeNode and downstream generation from that data structure to include in this PR as well. Would creating a project to track that change (and improvements like plumbing owner info for external repo files) help?

[brentleyjones]

I don't think we could take the change as is, since it would add to many additional groups to the project navigator.

In the example, just the ios_arm64-dbg-ios-arm64-min15 group will have to change, pointing to something including an owner based subpath instead of the path that it currently does. This probably will require an extra field on PathTreeNode (instead of the hack of isBazelGenerated = node.name.hasPrefix("bazel-out") that is currently being used, which btw would be a pretty big performance regression).

[sebastianv1]

since it would add to many additional groups to the project navigator.

Sounds good, I can take a look at removing the redundant groups. As another datapoint, I ran some numbers similar to @CognitiveDisson over our largest project and the number of overall groups increased by ~6.3% where the extraneous groups in question contributed to ~21% of the incremental groups added and just over 1% of the total project groups. Mileage obviously varies based on how much generated code your project contains (we have a decent amount).

instead of the hack of isBazelGenerated = node.name.hasPrefix("bazel-out") that is currently being used, which btw would be a pretty big performance regression

Do you have benchmarking numbers or infrastructure I can use to quantify this impact?

[brentleyjones]

Do you have benchmarking numbers or infrastructure I can use to quantify this impact?

I don't. In the past I had a company with a large project send over the inputs to this generator so I could profile and optimize it. the key thing is that we should do very few string comparisons, especially after we've processed the tree already.

[CognitiveDisson]

Is this the same issue @erikkerber posted above? Do you have an example I can use to repro 🙏 ?

@sebastianv1 Yes, it looks like the same problem. Unfortunately, I don't have an example. This issue can likely be reproduced with the local_repository rule.

[sebastianv1]

@brentleyjones Do you mind elaborating further on this comment and what you were thinking would be added to PathTreeNode?

just the ios_arm64-dbg-ios-arm64-min15 group will have to change, pointing to something including an owner based subpath instead of the path that it currently does. This probably will require an extra field on PathTreeNode (instead of the hack of isBazelGenerated = node.name.hasPrefix("bazel-out") that is currently being used

1. I check the prefix "bazel-out" the same as CreateRootElements.swift in order to reuse most of the same special group creation infrastructure laid out in ElementCreator.CreateSpecialRootGroupElement. This creates the parent Bazel Generated Files group of the configurations (ios_arm64-dbg-ios-arm64-min15), are you thinking we shouldn't need to reuse ElementCreator.CreateSpecialRootGroupElement?

2. For the PathTreeNode field, are you suggesting in calculatePathTree we modify the nodes' children of ios_arm64-dbg-ios-arm64-min15 and other configurations to instead point at the subpath of <owner> (i.e bin/<owner>/GenFile.swift) and then pass along the owner prefix bin/<owner> somehow in order to set the paths correctly inside the groups? Or were you thinking we leave calculatePathTree as I have it now, and modify group creation in CreateGroup to skip over the bin/<owner> children?

🙏 🙏

[brentleyjones]

Would you be fine with me making some changes and pushing them to this PR? I can look at it tonight.

[sebastianv1]

Not a problem! I don't suspect I'll make additional changes, but will let you know beforehand if I want to push anything

[brentleyjones]

Implemented with #3049.