build_file_content in new_local_repository overwrites original BUILD file

[PiotrSikora]

build_file_content in new_local_repository overwrites original BUILD file at the source and not just locally... build_file works as expected and overwritesBUILD file only locally, without modifying BUILD file at the source.

[katre]

If you already have a BUILD file, use local_repository, not new_local_repository.

[PiotrSikora]

I'm aware of local_repository, but sometimes BUILD file needs to be extended with custom targets.

Anyway, this seems like an inconsistency, since build_file doesn't overwrite the original BUILD file at the source.

[damienmg]

We should definitely not overwrite the source's BUILD file, I think this is happening because we write the BUILD file after symlinking everything from the source path. So if a build file exist we overwrite it, that is not good. build_file does symlinking so it does overwrite the symlink whereas build_file_content write the file itself.

[PiotrSikora]

Hey @kchodorow,
it looks that your fix broke existing behavior in which it was possible to use build_file_content with a repository that already had BUILD file, i.e. it was possible to provide alternative BUILD file.

// Make sure we're not overwriting an existing BUILD file.
if (buildFilePath.exists()) {
  Preconditions.checkState(buildFilePath.isSymbolicLink());
  buildFilePath.delete();
}

throws:

java.lang.RuntimeException: Unrecoverable error while evaluating node 'REPOSITORY_DIRECTORY:@ngx_brotli' (requested by nodes 'REPOSITORY:@ngx_brotli')
    at com.google.devtools.build.skyframe.ParallelEvaluator$Evaluate.run(ParallelEvaluator.java:447)
    at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:496)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalStateException
    at com.google.common.base.Preconditions.checkState(Preconditions.java:159)
    at com.google.devtools.build.lib.util.Preconditions.checkState(Preconditions.java:196)
    at com.google.devtools.build.lib.rules.repository.RepositoryFunction.writeBuildFile(RepositoryFunction.java:203)
    at com.google.devtools.build.lib.rules.repository.NewRepositoryBuildFileHandler.finishBuildFile(NewRepositoryBuildFileHandler.java:112)
    at com.google.devtools.build.lib.bazel.repository.NewGitRepositoryFunction.fetch(NewGitRepositoryFunction.java:42)
    at com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.compute(RepositoryDelegatorFunction.java:155)
    at com.google.devtools.build.skyframe.ParallelEvaluator$Evaluate.run(ParallelEvaluator.java:388)
    ... 4 more

[kchodorow]

Whoops. Thanks for the report.

[kchodorow]

Wait, how do you have a BUILD file in that directory that isn't a symbolic link? Could you do ls -l $(bazel info output_base)/external/ngx_brotli?

Edit: just realized that this is probably a remote repository, not a local one.

[PiotrSikora]

Yes, it's git repository.

$ ls -l $(bazel info output_base)/external/ngx_brotli
total 64
-rw-r--r--  1 piotrsikora  wheel  2330 Sep 23 12:45 BUILD
-rw-r--r--  1 piotrsikora  wheel  1466 Sep 23 12:45 CONTRIBUTING.md
-rw-r--r--  1 piotrsikora  wheel  1430 Sep 23 12:45 LICENSE
-rw-r--r--  1 piotrsikora  wheel  4833 Sep 23 12:45 README.md
-rw-r--r--  1 piotrsikora  wheel   113 Sep 23 12:45 WORKSPACE
-rw-r--r--  1 piotrsikora  wheel  5361 Sep 23 12:45 config
drwxr-xr-x  4 piotrsikora  wheel   136 Sep 23 12:45 src

[kchodorow]

Note: 2bc0939 should be cherry picked into the release (#1840).

[PiotrSikora]

Thanks! It works fine now.