-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add --experimental_repository_disable_download to allow users disable download for external repos #12940
Add --experimental_repository_disable_download to allow users disable download for external repos #12940
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ public class DownloadManager { | |
private List<Path> distdir = ImmutableList.of(); | ||
private UrlRewriter rewriter; | ||
private final Downloader downloader; | ||
private boolean disableDownload = false; | ||
|
||
public DownloadManager(RepositoryCache repositoryCache, Downloader downloader) { | ||
this.repositoryCache = repositoryCache; | ||
|
@@ -60,6 +61,10 @@ public void setUrlRewriter(UrlRewriter rewriter) { | |
this.rewriter = rewriter; | ||
} | ||
|
||
public void setDisableDownload(boolean disableDownload) { | ||
this.disableDownload = disableDownload; | ||
} | ||
|
||
/** | ||
* Downloads file to disk and returns path. | ||
* | ||
|
@@ -194,6 +199,10 @@ public Path download( | |
} | ||
} | ||
|
||
if (disableDownload) { | ||
throw new IOException(String.format("Failed to download repo %s: download is disabled.", repo)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add test cases to verify at least the following cases (
I'd love if this flag to also affected There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test cases added.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't think there is a lot we can do about that :( |
||
} | ||
|
||
try { | ||
downloader.download( | ||
urls, authHeaders, checksum, canonicalId, destination, eventHandler, clientEnv, type); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2061,4 +2061,92 @@ EOF | |
|| fail "Expected success despite needing a file behind basic auth" | ||
} | ||
|
||
function test_disable_download_should_prevent_downloading() { | ||
mkdir x | ||
echo 'exports_files(["file.txt"])' > x/BUILD | ||
echo 'Hello World' > x/file.txt | ||
tar cvf x.tar x | ||
sha256=$(sha256sum x.tar | head -c 64) | ||
serve_file x.tar | ||
|
||
mkdir main | ||
cd main | ||
cat > WORKSPACE <<EOF | ||
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") | ||
http_archive( | ||
name="ext", | ||
url = "http://127.0.0.1:$nc_port/x.tar", | ||
sha256="$sha256", | ||
) | ||
EOF | ||
cat > BUILD <<'EOF' | ||
genrule( | ||
name = "it", | ||
srcs = ["@ext//x:file.txt"], | ||
outs = ["it.txt"], | ||
cmd = "cp $< $@", | ||
) | ||
EOF | ||
|
||
bazel build --experimental_repository_disable_download //:it > "${TEST_log}" 2>&1 \ | ||
&& fail "Expected failure" || : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the first command failed (exit code is not 0), we need to use |
||
expect_log "Failed to download repo ext: download is disabled" | ||
} | ||
|
||
function test_disable_download_should_allow_distdir() { | ||
mkdir x | ||
echo 'exports_files(["file.txt"])' > x/BUILD | ||
echo 'Hello World' > x/file.txt | ||
tar cvf x.tar x | ||
sha256=$(sha256sum x.tar | head -c 64) | ||
|
||
mkdir main | ||
cp x.tar main | ||
cd main | ||
cat > WORKSPACE <<EOF | ||
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") | ||
http_archive( | ||
name="ext", | ||
url = "http://127.0.0.1/x.tar", | ||
sha256="$sha256", | ||
) | ||
EOF | ||
cat > BUILD <<'EOF' | ||
genrule( | ||
name = "it", | ||
srcs = ["@ext//x:file.txt"], | ||
outs = ["it.txt"], | ||
cmd = "cp $< $@", | ||
) | ||
EOF | ||
|
||
bazel build --distdir="." --experimental_repository_disable_download //:it || fail "Failed to build" | ||
} | ||
|
||
function test_disable_download_should_allow_local_repository() { | ||
mkdir x | ||
echo 'exports_files(["file.txt"])' > x/BUILD | ||
echo 'Hello World' > x/file.txt | ||
touch x/WORKSPACE | ||
|
||
mkdir main | ||
cd main | ||
cat > WORKSPACE <<EOF | ||
local_repository( | ||
name="ext", | ||
path="../x", | ||
) | ||
EOF | ||
cat > BUILD <<'EOF' | ||
genrule( | ||
name = "it", | ||
srcs = ["@ext//:file.txt"], | ||
outs = ["it.txt"], | ||
cmd = "cp $< $@", | ||
) | ||
EOF | ||
|
||
bazel build --experimental_repository_disable_download //:it || fail "Failed to build" | ||
} | ||
|
||
run_suite "local repository tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that this works because we don't provide any built-in repositories anymore that do downloading and we delegate everything to Starlark code instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works for repo rule which uses
repository_ctx.download
.