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

Nest output base inside of "outer" output path #1221

Merged

Conversation

brentleyjones
Copy link
Contributor

@brentleyjones brentleyjones commented Oct 4, 2022

This is to make sure that the analysis cache stays as hot as possible. This should bring the consistency of BwB performance more in line with BwX.

We currently don't set a different output base for each of the different configs (e.g. SwiftUI Preview, asan, etc.), as some generated build inputs (e.g. Info.plists) currently can't be found that way. This means that switching between SwiftUI Previews and normal builds, or having builds that have different sanitizers set, will clear at least some of the analysis cache.

@brentleyjones brentleyjones force-pushed the bj/nest-output-base-inside-of-outer-output-path branch 2 times, most recently from 7f65e66 to 538a5ae Compare October 5, 2022 12:36
@brentleyjones
Copy link
Contributor Author

brentleyjones commented Oct 5, 2022

This has an issue where for non-normal builds (e.g. SwiftUI Previews) non-source generated inputs, mainly plist files, will exist in a different place than XCBuild expects them. This isn't an issue for source files because of the bazel-out-overlay.yaml VFS overlay. I'm looking into that now, but I might have to revert that portion of this change.

Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Are there any particular projects or configurations that you'd like reviewers to validate with this change? I don't have any projects that use SwiftUI previews but I could try running SwiftLint with tsan with this if you like.

@brentleyjones
Copy link
Contributor Author

Because of the above bug, I think those builds might fail as well (but I'm working on those now). Just testing normal builds is good enough integration testing for me. I'm not sure how this one could fail, but it's a big enough change that I wanted some other people testing it out as well.

@maxwellE
Copy link
Contributor

maxwellE commented Oct 5, 2022

Top hatting this locally now

@brentleyjones
Copy link
Contributor Author

This currently breaks index importing as well, since the nested /execroot/ trips the regex used. I'm working on a fix for that as well. I probably will wait to merge this until after the 0.9 cut at this point though, so it has more time to bake.

@maxwellE
Copy link
Contributor

maxwellE commented Oct 5, 2022

Was able to test our targets with this and click to definition and syntax highlighting seem to be intact

@brentleyjones
Copy link
Contributor Author

(I also broke CI with this 😆.)

@brentleyjones brentleyjones force-pushed the bj/nest-output-base-inside-of-outer-output-path branch from 538a5ae to 3d7541b Compare October 5, 2022 19:06
This is to make sure that the analysis cache stays as hot as possible. This should bring the consistency of BwB performance more in line with BwX.

We currently don't set a different output base for each of the different configs (e.g. SwiftUI Preview, asan, etc.), as some generated build inputs (e.g. Info.plists) currently can't be found that way. This means that switching between SwiftUI Previews and normal builds, or having builds that have different sanitizers set, will clear at least some of the analysis cache.
@brentleyjones brentleyjones force-pushed the bj/nest-output-base-inside-of-outer-output-path branch from 470d859 to 927e13d Compare October 6, 2022 19:17
@brentleyjones brentleyjones merged commit 1e95b0d into main Oct 6, 2022
@brentleyjones brentleyjones deleted the bj/nest-output-base-inside-of-outer-output-path branch October 6, 2022 19:37
@erikkerber
Copy link
Contributor

I'm consistently getting a Bazel crash after this commit. Both 5.3.1 and 6.0.0-pre.20220922.1

DEBUG: Repository com_github_buildbuddy_io_rules_xcodeproj instantiated at:
  /Users/ekerber/dev/slack/slack-objc/WORKSPACE:220:12: in <toplevel>
  /Users/ekerber/dev/slack/slack-objc/bazel/github_repo.bzl:28:17: in github_repo
Repository rule http_archive defined at:
  /private/var/tmp/_bazel_ekerber/dd323d2a4ff53820c11ccc0d5aebe791/execroot/slack-objc/bazel-out/_rules_xcodeproj/build_output_base/external/bazel_tools/tools/build_defs/repo/http.bzl:370:31: in <toplevel>
Loading:
Loading: 1 packages loaded
Analyzing: target //App:Slack-BwB.generator (2 packages loaded, 0 targets configured)
Analyzing: target //App:Slack-BwB.generator (310 packages loaded, 6 targets configured)
Analyzing: target //App:Slack-BwB.generator (337 packages loaded, 63 targets configured)
Analyzing: target //App:Slack-BwB.generator (338 packages loaded, 63 targets configured)
Analyzing: target //App:Slack-BwB.generator (338 packages loaded, 63 targets configured)
Analyzing: target //App:Slack-BwB.generator (352 packages loaded, 1241 targets configured)
Analyzing: target //App:Slack-BwB.generator (357 packages loaded, 1268 targets configured)
Analyzing: target //App:Slack-BwB.generator (357 packages loaded, 1268 targets configured)
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.RuntimeException: Unrecoverable error while evaluating node 'REPOSITORY_DIRECTORY:@WORKSPACE' (requested by nodes '[<absolute root>]/[/private/var/tmp/_bazel_ekerber/dd323d2a4ff53820c11ccc0d5aebe791/execroot/slack-objc/bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE]')
        at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:646)
        at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:382)
        at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
        at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
        at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
        at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
        at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.lang.ClassCastException: class com.google.devtools.build.lib.packages.InputFile cannot be cast to class com.google.devtools.build.lib.packages.Rule (com.google.devtools.build.lib.packages.InputFile and com.google.devtools.build.lib.packages.Rule are in unnamed module of loader 'app')
        at com.google.devtools.build.lib.packages.Package.getRule(Package.java:648)
        at com.google.devtools.build.lib.repository.ExternalPackageHelper$ExternalPackageRuleExtractor.processAndShouldContinue(ExternalPackageHelper.java:144)
        at com.google.devtools.build.lib.repository.ExternalPackageHelper.iterateWorkspaceFragments(ExternalPackageHelper.java:118)
        at com.google.devtools.build.lib.repository.ExternalPackageHelper.getRuleByName(ExternalPackageHelper.java:52)
        at com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.getRepoRuleFromWorkspace(RepositoryDelegatorFunction.java:446)
        at com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.compute(RepositoryDelegatorFunction.java:285)
        at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:572)
        ... 7 more

@brentleyjones
Copy link
Contributor Author

brentleyjones commented Oct 10, 2022

Where do you get this crash? Do you still get the crash after a bazel clean --expunge? How about after deleting the bazel-* symlinks?

For some reason Bazel is looking at those files as inputs. Looks similar to bazelbuild/bazel#10653 and bazelbuild/bazel#13951.

@brentleyjones
Copy link
Contributor Author

Also, there shouldn't be (and probably isn't?) a WORKSPACE at bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE. If there is a file there, let me know, because that could be an issue as well.

@erikkerber
Copy link
Contributor

erikkerber commented Oct 10, 2022

Where do you get this crash?

It seems to happen during analysis of project generation

https://app.buildbuddy.io/invocation/d4a6721d-fd10-482b-9cd7-9d6f0862746f

Generating "Slack-BwB.xcodeproj"
...
Analyzing: target //App:Slack-BwB.generator (359 packages loaded, 1524 targets configured)
Analyzing: target //App:Slack-BwB.generator (359 packages loaded, 1524 targets configured)
FATAL: bazel crashed due to an internal error. Printing stack trace:

Do you still get the crash after a bazel clean --expunge?

Yes

How about after deleting the bazel-* symlinks?

Same

Also, there shouldn't be (and probably isn't?) a WORKSPACE at bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE

❯ file bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE
bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE: cannot open `bazel-out/_rules_xcodeproj/build_output_base/external/WORKSPACE' (No such file or directory)

@brentleyjones
Copy link
Contributor Author

Are you able to create a minimal repro for this? The example projects don't hit this (even when unsetting the output_base override). I'm wondering if you have a flag set that is changing the searching behavior in some way.

@erikkerber
Copy link
Contributor

I was able to work around this for now by setting a custom/relative --output_base like similar to the setup in this repo.

Is that intended to be a requirement? I think not having it relative leads to an infinite symlink (though real root of that I haven't looked very deeply into, for instance if there is a bad assumption being made)

@brentleyjones
Copy link
Contributor Author

It's not a requirement, and I test without it as well. Can you create a minimal repro for me (even showing how the examples get in that state)?

@brentleyjones
Copy link
Contributor Author

I think the main issue might be that we have build --experimental_convenience_symlinks=ignore set in the repo. Without that I can see the convenience symlinks getting in the way. I'll try to repro this.

@brentleyjones
Copy link
Contributor Author

#1264

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.

4 participants