From fee8d254c79628a63c8904dfaf904edf7ebbe760 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 22 Aug 2018 10:37:09 -0700 Subject: [PATCH] Third cl for verbose workspaces (ability to log certain potentially non-hermetic events that happen as part of repository rules). Adding logging for download and download_and_extract In the future: - Add more event types - Allowing to specify log file rather than dumping to INFO - Log levels, full or alerts only RELNOTES: None PiperOrigin-RevId: 209789459 --- .../lib/bazel/debug/WorkspaceRuleEvent.java | 64 +++++- .../build/lib/bazel/debug/workspace_log.proto | 27 +++ .../skylark/SkylarkRepositoryContext.java | 20 +- .../SkylarkRepositoryContextApi.java | 216 +++++++++--------- src/test/shell/bazel/bazel_workspaces_test.sh | 139 ++++++++++- 5 files changed, 346 insertions(+), 120 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/debug/WorkspaceRuleEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/debug/WorkspaceRuleEvent.java index 38c9da3e7c87c0..2ff1bb513a7d7e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/debug/WorkspaceRuleEvent.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/debug/WorkspaceRuleEvent.java @@ -16,6 +16,8 @@ import com.google.devtools.build.lib.bazel.debug.proto.WorkspaceLogProtos; import com.google.devtools.build.lib.events.ExtendedEventHandler.ProgressLike; import com.google.devtools.build.lib.events.Location; +import java.net.URL; +import java.util.List; import java.util.Map; /** An event to record events happening during workspace rule resolution */ @@ -32,8 +34,6 @@ private WorkspaceRuleEvent(WorkspaceLogProtos.WorkspaceEvent event) { /** * Creates a new WorkspaceRuleEvent for an execution event. - * - *

Note: we will add more granular information as needed. */ public static WorkspaceRuleEvent newExecuteEvent( Iterable args, @@ -73,6 +73,66 @@ public static WorkspaceRuleEvent newExecuteEvent( return new WorkspaceRuleEvent(result.build()); } + /** Creates a new WorkspaceRuleEvent for a download event. */ + public static WorkspaceRuleEvent newDownloadEvent( + List urls, + String output, + String sha256, + Boolean executable, + String ruleLabel, + Location location) { + WorkspaceLogProtos.DownloadEvent.Builder e = + WorkspaceLogProtos.DownloadEvent.newBuilder() + .setOutput(output) + .setSha256(sha256) + .setExecutable(executable); + for (URL u : urls) { + e.addUrl(u.toString()); + } + + WorkspaceLogProtos.WorkspaceEvent.Builder result = + WorkspaceLogProtos.WorkspaceEvent.newBuilder(); + result = result.setDownloadEvent(e.build()); + if (location != null) { + result = result.setLocation(location.print()); + } + if (ruleLabel != null) { + result = result.setRule(ruleLabel); + } + return new WorkspaceRuleEvent(result.build()); + } + + /** Creates a new WorkspaceRuleEvent for a download event. */ + public static WorkspaceRuleEvent newDownloadAndExtractEvent( + List urls, + String output, + String sha256, + String type, + String stripPrefix, + String ruleLabel, + Location location) { + WorkspaceLogProtos.DownloadAndExtractEvent.Builder e = + WorkspaceLogProtos.DownloadAndExtractEvent.newBuilder() + .setOutput(output) + .setSha256(sha256) + .setType(type) + .setStripPrefix(stripPrefix); + for (URL u : urls) { + e.addUrl(u.toString()); + } + + WorkspaceLogProtos.WorkspaceEvent.Builder result = + WorkspaceLogProtos.WorkspaceEvent.newBuilder(); + result = result.setDownloadAndExtractEvent(e.build()); + if (location != null) { + result = result.setLocation(location.print()); + } + if (ruleLabel != null) { + result = result.setRule(ruleLabel); + } + return new WorkspaceRuleEvent(result.build()); + } + /* * @return a message to log for this event */ diff --git a/src/main/java/com/google/devtools/build/lib/bazel/debug/workspace_log.proto b/src/main/java/com/google/devtools/build/lib/bazel/debug/workspace_log.proto index 1dea56af9cbe82..edab4fa463408f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/debug/workspace_log.proto +++ b/src/main/java/com/google/devtools/build/lib/bazel/debug/workspace_log.proto @@ -38,6 +38,31 @@ message ExecuteEvent { string output_directory = 6; } +// Information on "Download" event in repository_ctx. +message DownloadEvent { + // Url to download from. If multiple, treated as mirrors + repeated string url = 1; + // Output file + string output = 2; + // sha256, if speficied + string sha256 = 3; + // whether to make the resulting file executable + bool executable = 4; +} + +message DownloadAndExtractEvent { + // Url(s) to download from + repeated string url = 1; + // Output file + string output = 2; + // sha256, if specified + string sha256 = 3; + // Archive type, if specified. Otherwise, inferred from URL. + string type = 4; + // A directory prefix to strip from extracted files. + string strip_prefix = 5; +} + message WorkspaceEvent { // Location in the code (.bzl file) where the event originates. string location = 1; @@ -47,5 +72,7 @@ message WorkspaceEvent { oneof event { ExecuteEvent execute_event = 3; + DownloadEvent download_event = 4; + DownloadAndExtractEvent download_and_extract_event = 5; } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java index a618eaedb78908..0aaebe10150b9c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java @@ -298,11 +298,16 @@ private SkylarkPath findCommandOnPath(String program) throws IOException { } @Override - public void download(Object url, Object output, String sha256, Boolean executable) + public void download( + Object url, Object output, String sha256, Boolean executable, Location location) throws RepositoryFunctionException, EvalException, InterruptedException { validateSha256(sha256); List urls = getUrls(url); SkylarkPath outputPath = getPath("download()", output); + WorkspaceRuleEvent w = + WorkspaceRuleEvent.newDownloadEvent( + urls, output.toString(), sha256, executable, rule.getLabel().toString(), location); + env.getListener().post(w); try { checkInOutputDirectory(outputPath); makeDirectories(outputPath.getPath()); @@ -326,11 +331,22 @@ public void download(Object url, Object output, String sha256, Boolean executabl @Override public void downloadAndExtract( - Object url, Object output, String sha256, String type, String stripPrefix) + Object url, Object output, String sha256, String type, String stripPrefix, Location location) throws RepositoryFunctionException, InterruptedException, EvalException { validateSha256(sha256); List urls = getUrls(url); + WorkspaceRuleEvent w = + WorkspaceRuleEvent.newDownloadAndExtractEvent( + urls, + output.toString(), + sha256, + type, + stripPrefix, + rule.getLabel().toString(), + location); + env.getListener().post(w); + // Download to outputDirectory and delete it after extraction SkylarkPath outputPath = getPath("download_and_extract()", output); checkInOutputDirectory(outputPath); diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/repository/SkylarkRepositoryContextApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/repository/SkylarkRepositoryContextApi.java index 698f307ba0f735..900fe0a4fa1b16 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/repository/SkylarkRepositoryContextApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/repository/SkylarkRepositoryContextApi.java @@ -251,120 +251,114 @@ public SkylarkExecutionResultApi execute( public RepositoryPathApi which(String program) throws EvalException; @SkylarkCallable( - name = "download", - doc = "Download a file to the output path for the provided url.", - parameters = { - @Param( - name = "url", - allowedTypes = { - @ParamType(type = String.class), - @ParamType(type = Iterable.class, generic1 = String.class), - }, - named = true, - doc = "List of mirror URLs referencing the same file." - ), - @Param( - name = "output", - allowedTypes = { - @ParamType(type = String.class), - @ParamType(type = Label.class), - @ParamType(type = RepositoryPathApi.class) - }, - defaultValue = "''", - named = true, - doc = "path to the output file, relative to the repository directory." - ), - @Param( - name = "sha256", - type = String.class, - defaultValue = "''", - named = true, - doc = - "the expected SHA-256 hash of the file downloaded." - + " This must match the SHA-256 hash of the file downloaded. It is a security risk" - + " to omit the SHA-256 as remote files can change. At best omitting this field" - + " will make your build non-hermetic. It is optional to make development easier" - + " but should be set before shipping." - ), - @Param( - name = "executable", - type = Boolean.class, - defaultValue = "False", - named = true, - doc = "set the executable flag on the created file, false by default." - ), - } - ) - public void download(Object url, Object output, String sha256, Boolean executable) + name = "download", + doc = "Download a file to the output path for the provided url.", + useLocation = true, + parameters = { + @Param( + name = "url", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Iterable.class, generic1 = String.class), + }, + named = true, + doc = "List of mirror URLs referencing the same file."), + @Param( + name = "output", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Label.class), + @ParamType(type = RepositoryPathApi.class) + }, + defaultValue = "''", + named = true, + doc = "path to the output file, relative to the repository directory."), + @Param( + name = "sha256", + type = String.class, + defaultValue = "''", + named = true, + doc = + "the expected SHA-256 hash of the file downloaded." + + " This must match the SHA-256 hash of the file downloaded. It is a security" + + " risk to omit the SHA-256 as remote files can change. At best omitting this" + + " field will make your build non-hermetic. It is optional to make" + + " development easier but should be set before shipping."), + @Param( + name = "executable", + type = Boolean.class, + defaultValue = "False", + named = true, + doc = "set the executable flag on the created file, false by default."), + }) + public void download( + Object url, Object output, String sha256, Boolean executable, Location location) throws RepositoryFunctionExceptionT, EvalException, InterruptedException; @SkylarkCallable( - name = "download_and_extract", - doc = "Download a file to the output path for the provided url, and extract it.", - parameters = { - @Param( - name = "url", - allowedTypes = { - @ParamType(type = String.class), - @ParamType(type = Iterable.class, generic1 = String.class), - }, - named = true, - doc = "List of mirror URLs referencing the same file." - ), - @Param( - name = "output", - allowedTypes = { - @ParamType(type = String.class), - @ParamType(type = Label.class), - @ParamType(type = RepositoryPathApi.class) - }, - defaultValue = "''", - named = true, - doc = - "path to the directory where the archive will be unpacked," - + " relative to the repository directory." - ), - @Param( - name = "sha256", - type = String.class, - defaultValue = "''", - named = true, - doc = - "the expected SHA-256 hash of the file downloaded." - + " This must match the SHA-256 hash of the file downloaded. It is a security risk" - + " to omit the SHA-256 as remote files can change. At best omitting this field" - + " will make your build non-hermetic. It is optional to make development easier" - + " but should be set before shipping." - + " If provided, the repository cache will first be checked for a file with the" - + " given hash; a download will only be attempted, if the file was not found in the" - + " cache. After a successful download, the file will be added to the cache." - ), - @Param( - name = "type", - type = String.class, - defaultValue = "''", - named = true, - doc = - "the archive type of the downloaded file." - + " By default, the archive type is determined from the file extension of the URL." - + " If the file has no extension, you can explicitly specify either \"zip\"," - + " \"jar\", \"war\", \"tar.gz\", \"tgz\", \"tar.bz2\", or \"tar.xz\" here." - ), - @Param( - name = "stripPrefix", - type = String.class, - defaultValue = "''", - named = true, - doc = - "a directory prefix to strip from the extracted files." - + "\nMany archives contain a top-level directory that contains all files in the" - + " archive. Instead of needing to specify this prefix over and over in the" - + " build_file, this field can be used to strip it from extracted" - + " files." - ), - } - ) + name = "download_and_extract", + doc = "Download a file to the output path for the provided url, and extract it.", + useLocation = true, + parameters = { + @Param( + name = "url", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Iterable.class, generic1 = String.class), + }, + named = true, + doc = "List of mirror URLs referencing the same file."), + @Param( + name = "output", + allowedTypes = { + @ParamType(type = String.class), + @ParamType(type = Label.class), + @ParamType(type = RepositoryPathApi.class) + }, + defaultValue = "''", + named = true, + doc = + "path to the directory where the archive will be unpacked," + + " relative to the repository directory."), + @Param( + name = "sha256", + type = String.class, + defaultValue = "''", + named = true, + doc = + "the expected SHA-256 hash of the file downloaded." + + " This must match the SHA-256 hash of the file downloaded. It is a security" + + " risk to omit the SHA-256 as remote files can change. At best omitting this" + + " field will make your build non-hermetic. It is optional to make" + + " development easier but should be set before shipping." + + " If provided, the repository cache will first be checked for a file with the" + + " given hash; a download will only be attempted, if the file was not found" + + " in the cache. After a successful download, the file will be added to the" + + " cache."), + @Param( + name = "type", + type = String.class, + defaultValue = "''", + named = true, + doc = + "the archive type of the downloaded file." + + " By default, the archive type is determined from the file extension of the" + + " URL. If the file has no extension, you can explicitly specify either" + + " \"zip\", \"jar\", \"war\", \"tar.gz\", \"tgz\", \"tar.bz2\", or \"tar.xz\"" + + " here."), + @Param( + name = "stripPrefix", + type = String.class, + defaultValue = "''", + named = true, + doc = + "a directory prefix to strip from the extracted files." + + "\nMany archives contain a top-level directory that contains all files in the" + + " archive. Instead of needing to specify this prefix over and over in the" + + " build_file, this field can be used to strip it from extracted" + + " files."), + }) public void downloadAndExtract( - Object url, Object output, String sha256, String type, String stripPrefix) + Object url, Object output, String sha256, String type, String stripPrefix, Location location) throws RepositoryFunctionExceptionT, InterruptedException, EvalException; } diff --git a/src/test/shell/bazel/bazel_workspaces_test.sh b/src/test/shell/bazel/bazel_workspaces_test.sh index 75ebc783926162..efad56ea328018 100755 --- a/src/test/shell/bazel/bazel_workspaces_test.sh +++ b/src/test/shell/bazel/bazel_workspaces_test.sh @@ -20,15 +20,17 @@ CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "${CURRENT_DIR}/../integration_test_setup.sh" \ || { echo "integration_test_setup.sh not found!" >&2; exit 1; } +source "${CURRENT_DIR}/remote_helpers.sh" \ + || { echo "remote_helpers.sh not found!" >&2; exit 1; } function test_execute() { create_new_workspace - cat > BUILD < BUILD <<'EOF' genrule( name="test", srcs=["@repo//:t.txt"], outs=["out.txt"], - cmd="echo Result > \$(location out.txt)" + cmd="echo Result > $(location out.txt)" ) EOF cat >> repos.bzl < output || fail "could not build //:test" - cat output &> $TEST_log + cat output >> $TEST_log executes=`grep "location: .*repos.bzl:2:3" output | wc -l` if [ "$executes" -ne "0" ] then @@ -68,12 +70,12 @@ EOF function test_reexecute() { create_new_workspace - cat > BUILD < BUILD <<'EOF' genrule( name="test", srcs=["@repo//:t.txt"], outs=["out.txt"], - cmd="echo Result > \$(location out.txt)" + cmd="echo Result > $(location out.txt)" ) EOF cat >> repos.bzl < BUILD <<'EOF' +genrule( + name="test", + srcs=["@repo//:t.txt"], + outs=["out.txt"], + cmd="echo Result > $(location out.txt)" +) +EOF + cat >> repos.bzl <> WORKSPACE < ${TEST_log} || fail "could not build //:test\n" + expect_log "location: .*repos.bzl:2:3" + expect_log "arguments: \"echo\"" + expect_log "arguments: \"test_contents\"" + expect_log "timeout_seconds: 21" + expect_log "quiet: true" + expect_log "key: \"Arg1\"" + expect_log "value: \"Val1\"" + expect_log "rule: \"//external:repo\"" +} + + +function test_download() { + # Prepare HTTP server with Python + local server_dir="${TEST_TMPDIR}/server_dir" + mkdir -p "${server_dir}" + local file="${server_dir}/file.txt" + echo "file contents here" > "${file}" + file_sha256="$(sha256sum "${file}" | head -c 64)" + + # Start HTTP server with Python + startup_server "${server_dir}" + + set_workspace_command "repository_ctx.download(\"http://localhost:${fileserver_port}/file.txt\", \"file.txt\", \"${file_sha256}\")" + + bazel build //:test --experimental_workspace_rules_logging=yes &> ${TEST_log} && shutdown_server || fail "could not build //:test\n" + expect_log "location: .*repos.bzl:2:3" + expect_log "rule: \"//external:repo\"" + expect_log "download_event" + expect_log "url: \"http://localhost:${fileserver_port}/file.txt\"" + expect_log "output: \"file.txt\"" + expect_log "sha256: \"${file_sha256}\"" +} + +function test_download_multiple() { + # Prepare HTTP server with Python + local server_dir="${TEST_TMPDIR}/server_dir" + mkdir -p "${server_dir}" + local file2="${server_dir}/file2.txt" + echo "second contents here" > "${file2}" + + # Start HTTP server with Python + startup_server "${server_dir}" + + set_workspace_command "repository_ctx.download([\"http://localhost:${fileserver_port}/file1.txt\",\"http://localhost:${fileserver_port}/file2.txt\"], \"out_for_list.txt\")" + + bazel build //:test --experimental_workspace_rules_logging=yes &> $TEST_log && shutdown_server || fail "could not build //:test\n" + expect_log "location: .*repos.bzl:2:3" + expect_log "rule: \"//external:repo\"" + expect_log "download_event" + expect_log "url: \"http://localhost:${fileserver_port}/file1.txt\"" + expect_log "url: \"http://localhost:${fileserver_port}/file2.txt\"" + expect_log "output: \"out_for_list.txt\"" +} + +function test_download_and_extract() { + # Prepare HTTP server with Python + local server_dir="${TEST_TMPDIR}/server_dir" + mkdir -p "${server_dir}" + local file_prefix="${server_dir}/download_and_extract" + + pushd ${TEST_TMPDIR} + echo "This is one file" > server_dir/download_and_extract.txt + zip -r server_dir/download_and_extract.zip server_dir + file_sha256="$(sha256sum server_dir/download_and_extract.zip | head -c 64)" + popd + + # Start HTTP server with Python + startup_server "${server_dir}" + + set_workspace_command "repository_ctx.download_and_extract(\"http://localhost:${fileserver_port}/download_and_extract.zip\", \"out_dir\", \"${file_sha256}\", \"zip\", \"server_dir/\")" + + bazel build //:test --experimental_workspace_rules_logging=yes &> ${TEST_log} && shutdown_server || fail "could not build //:test\n" + + expect_log "location: .*repos.bzl:2:3" + expect_log "rule: \"//external:repo\"" + expect_log "download_and_extract_event" + expect_log "url: \"http://localhost:${fileserver_port}/download_and_extract.zip\"" + expect_log "output: \"out_dir\"" + expect_log "sha256: \"${file_sha256}\"" + expect_log "type: \"zip\"" + expect_log "strip_prefix: \"server_dir/\"" +} + +function tear_down() { + shutdown_server + if [ -d "${TEST_TMPDIR}/server_dir" ]; then + rm -fr "${TEST_TMPDIR}/server_dir" + fi + true +} + run_suite "workspaces_tests"