Skip to content

Only move indexstores generated by build#78

Merged
amberdixon merged 3 commits intomasterfrom
amber/move-incremental-indexstores
Jun 24, 2020
Merged

Only move indexstores generated by build#78
amberdixon merged 3 commits intomasterfrom
amber/move-incremental-indexstores

Conversation

@amberdixon
Copy link
Collaborator

Use BEP to determine which indexstore paths were generated by the compilation steps of the build. Only move those over, so we don't move every generated indexstore. This was a suggestion from other engineers at ios-dev-at-scale to make index remapping more efficient.

@amberdixon amberdixon requested review from ndizazzo and segiddins June 17, 2020 17:08
do
if [ -d $i ]
then
EXISTING_INDEXSTORES+=($i)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For simpler projects (like the ones in this repo), no unit files and no indexstore directory will be generated. In that case, we shouldn't try to move non-existent directories!

Copy link
Member

Choose a reason for hiding this comment

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

For simpler projects (like the ones in this repo), no unit files and no indexstore directory will be generated

If that's the case, can you please add a new project that will exercise this code path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize now that the unit files were not being generated because sandboxing was enabled. Bazel would build the outputs and then delete any other files left behind (including the generated indexstores.)

For now, if anyone would like to manually test index generation in the rules_ios repo, just make sure to disable sandboxing by adding these lines to .bazelrc:

build --genrule_strategy=standalone
build --spawn_strategy=standalone

set -euxo pipefail

$BAZEL_PATH build $1 2>&1 | $BAZEL_OUTPUT_PROCESSOR
$BAZEL_PATH build --build_event_text_file=$bazel_build_event_text_filename --build_event_publish_all_actions $1 2>&1 | $BAZEL_OUTPUT_PROCESSOR
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered having the output-processor read from the bazel build event text file instead. But then I realized that we want to show error messages in the output as they appear, rather than waiting until the build is finished.

Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

What is the performance impact of this change on a large project? How does it compare to MobileNativeFoundation/index-import#46 ?

set -euxo pipefail
cd $BAZEL_WORKSPACE_ROOT

export bazel_build_event_text_filename=$(mktemp -d)/build_event.txt
Copy link
Member

Choose a reason for hiding this comment

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

since this is exported as an env var, capitalize?

Copy link
Member

Choose a reason for hiding this comment

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

also, should the tmp dir be cleaned up after the installer gets run? Does it make sense to instead use one of Xcode's managed tmp dirs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, this file is not very large and I do sometimes look at it after the build is over. I would prefer to keep it around.

Could you clarify what you mean by one of "xcode's managed tmp dirs"? I'm not sure if I've heard of this before.

Copy link
Member

Choose a reason for hiding this comment

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

Something like https://pewpewthespells.com/blog/buildsettings.html#derived_file_dir, or one of the other TEMP directories documented on that page

| xargs -0 "$BAZEL_INSTALLERS_DIR/_indexstore.sh"

echo "Finish remapping index files" No newline at end of file
echo "Start remapping index files at `date`"
Copy link
Member

Choose a reason for hiding this comment

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

debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put that in here intentionally, because it made it easy for me to look inside the bazel-xcode-diagnostics directory to see if my index remapping was in progress.

do
if [ -d $i ]
then
EXISTING_INDEXSTORES+=($i)
Copy link
Member

Choose a reason for hiding this comment

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

For simpler projects (like the ones in this repo), no unit files and no indexstore directory will be generated

If that's the case, can you please add a new project that will exercise this code path?

@amberdixon
Copy link
Collaborator Author

What is the performance impact of this change on a large project? How does it compare to lyft/index-import#46 ?

I tried this on a larger project within Square. When I did a clean build, 111 indexstores were generated and remapped in 2 seconds. On incremental builds where no source files are changed, 0 indexstores are moved. For some reason, the objc indexstore didn't always get generated however, and that is likely the largest one.

@amberdixon amberdixon merged commit d81b490 into master Jun 24, 2020
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.

2 participants