From 09ff174a7125c115ea7fd476104d53bcf2c493b5 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Tue, 12 Sep 2023 13:47:38 -0700 Subject: [PATCH 01/16] feat: add support for arbitrary headers rctx.download[_and_extract] --- .../starlark/StarlarkBaseExternalContext.java | 74 ++++++++++++++++++- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index e9b641086aa33d..cd5f6d3a644c76 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -223,6 +223,46 @@ private static ImmutableMap>> getAuthHeaders( return res; } + private static ImmutableMap>> getHeaders( + Map>> auth) throws RepositoryFunctionException, EvalException { + ImmutableMap.Builder>> headers = new ImmutableMap.Builder<>(); + for (Map.Entry>> entry : auth.entrySet()) { + try { + URL url = new URL(entry.getKey()); + Map> headerMap = entry.getValue(); + headers.put(url.toURI(), headerMap); + } catch (MalformedURLException e) { + throw new RepositoryFunctionException(e, Transience.PERSISTENT); + } catch (URISyntaxException e) { + throw new EvalException(e); + } + } + return headers.buildOrThrow(); + } + + private static ImmutableMap>> getHeaderContents(Dict x, String what) + throws EvalException { + // Dict.cast returns Dict. + @SuppressWarnings({"unchecked", "rawtypes"}) + Map> urlHeaderMapUnchecked = (Map) Dict.cast(x, String.class, Dict.class, what); + + ImmutableMap.Builder>> urlHeadersMap = new ImmutableMap.Builder<>(); + + for (Map.Entry> urlHeaderEntry : urlHeaderMapUnchecked.entrySet()) { + + ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); + Dict headersUnchecked = Dict.cast(urlHeaderEntry.getValue(), String.class, List.class, "header entry"); + + for (Map.Entry headerEntry : headersUnchecked.entrySet()) { + List headerValue = headerEntry.getValue().stream().map(r -> r.toString()).toList(); + headers.put(headerEntry.getKey(), headerValue); + } + + urlHeadersMap.put(urlHeaderEntry.getKey(), headers.buildOrThrow()); + } + return urlHeadersMap.buildOrThrow(); +} + private static ImmutableList checkAllUrls(Iterable urlList) throws EvalException { ImmutableList.Builder result = ImmutableList.builder(); @@ -424,6 +464,11 @@ private StructImpl calculateDownloadResult(Optional checksum, Path dow defaultValue = "{}", named = true, doc = "An optional dict specifying authentication information for some of the URLs."), + @Param( + name = "headers", + defaultValue = "{}", + named = true, + doc = "An optional dict specifying http headers for some of the URLs."), @Param( name = "integrity", defaultValue = "''", @@ -444,12 +489,20 @@ public StructImpl download( Boolean allowFail, String canonicalId, Dict authUnchecked, // expected + Dict headersUnchecked, // expected String integrity, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { ImmutableMap>> authHeaders = getAuthHeaders(getAuthContents(authUnchecked, "auth")); + ImmutableMap>> headers = getHeaders(getHeaderContents(headersUnchecked, "headers")); + + ImmutableMap>> allHeaders = new ImmutableMap.Builder>>() + .putAll(authHeaders) + .putAll(headers) + .buildOrThrow(); + ImmutableList urls = getUrls( url, @@ -483,7 +536,7 @@ public StructImpl download( downloadedPath = downloadManager.download( urls, - authHeaders, + allHeaders, checksum, canonicalId, Optional.empty(), @@ -598,6 +651,11 @@ public StructImpl download( defaultValue = "{}", named = true, doc = "An optional dict specifying authentication information for some of the URLs."), + @Param( + name = "headers", + defaultValue = "{}", + named = true, + doc = "An optional dict specifying http headers for some of the URLs."), @Param( name = "integrity", defaultValue = "''", @@ -629,13 +687,21 @@ public StructImpl downloadAndExtract( String stripPrefix, Boolean allowFail, String canonicalId, - Dict auth, // expected + Dict authUnchecked, // expected + Dict headersUnchecked, // expected String integrity, Dict renameFiles, // expected StarlarkThread thread) throws RepositoryFunctionException, InterruptedException, EvalException { ImmutableMap>> authHeaders = - getAuthHeaders(getAuthContents(auth, "auth")); + getAuthHeaders(getAuthContents(authUnchecked, "auth")); + + ImmutableMap>> headers = getHeaders(getHeaderContents(headersUnchecked, "headers")); + + ImmutableMap>> allHeaders = new ImmutableMap.Builder>>() + .putAll(authHeaders) + .putAll(headers) + .buildOrThrow(); ImmutableList urls = getUrls( @@ -684,7 +750,7 @@ public StructImpl downloadAndExtract( downloadedPath = downloadManager.download( urls, - authHeaders, + allHeaders, checksum, canonicalId, Optional.of(type), From e89950dc08db918c18b774e1fb132dbb2f8e9ae7 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Wed, 25 Oct 2023 14:35:03 -0700 Subject: [PATCH 02/16] refactor --- .../downloader/DelegatingDownloader.java | 3 +- .../downloader/DownloadManager.java | 2 + .../repository/downloader/Downloader.java | 1 + .../downloader/HttpConnectorMultiplexer.java | 10 ++- .../repository/downloader/HttpDownloader.java | 7 ++- .../starlark/StarlarkBaseExternalContext.java | 62 +++++-------------- .../downloader/GrpcRemoteDownloader.java | 3 +- 7 files changed, 33 insertions(+), 55 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java index 249cc22c7c0d19..129a730bcfc8b6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DelegatingDownloader.java @@ -47,6 +47,7 @@ public void setDelegate(@Nullable Downloader delegate) { @Override public void download( List urls, + Map> headers, Credentials credentials, Optional checksum, String canonicalId, @@ -60,6 +61,6 @@ public void download( downloader = delegate; } downloader.download( - urls, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); + urls, headers, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index ae9fe3543a2806..ac3e49e04476cf 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -122,6 +122,7 @@ public void setCredentialFactory(CredentialFactory credentialFactory) { */ public Path download( List originalUrls, + Map> headers, Map>> authHeaders, Optional checksum, String canonicalId, @@ -267,6 +268,7 @@ public Path download( try { downloader.download( rewrittenUrls, + headers, credentialFactory.create(rewrittenAuthHeaders), checksum, canonicalId, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java index 1e8fc932b43b08..79a0076a3f56e1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Downloader.java @@ -42,6 +42,7 @@ public interface Downloader { */ void download( List urls, + Map> headers, Credentials credentials, Optional checksum, String canonicalId, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index 1d5ba9b08a435c..72ef98afe59575 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -75,7 +75,7 @@ final class HttpConnectorMultiplexer { } public HttpStream connect(URL url, Optional checksum) throws IOException { - return connect(url, checksum, StaticCredentials.EMPTY, Optional.empty()); + return connect(url, checksum, Map.of(), StaticCredentials.EMPTY, Optional.empty()); } /** @@ -96,14 +96,18 @@ public HttpStream connect(URL url, Optional checksum) throws IOExcepti * @throws IllegalArgumentException if {@code urls} is empty or has an unsupported protocol */ public HttpStream connect( - URL url, Optional checksum, Credentials credentials, Optional type) + URL url, Optional checksum, Map> headers, Credentials credentials, Optional type) throws IOException { Preconditions.checkArgument(HttpUtils.isUrlSupportedByDownloader(url)); if (Thread.interrupted()) { throw new InterruptedIOException(); } + Map> baseHeaders = new HashMap<>(headers); + // REQUEST_HEADERS should not be overriable by user provided headers + baseHeaders.putAll(REQUEST_HEADERS); + Function>> headerFunction = - getHeaderFunction(REQUEST_HEADERS, credentials); + getHeaderFunction(baseHeaders, credentials); URLConnection connection = connector.connect(url, headerFunction); return httpStreamFactory.create( connection, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java index 1cc14c1dd183c8..6464aad4caeffd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java @@ -15,6 +15,8 @@ package com.google.devtools.build.lib.bazel.repository.downloader; import com.google.auth.Credentials; +import com.google.common.base.Optional; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.io.ByteStreams; @@ -74,6 +76,7 @@ public void setMaxRetryTimeout(Duration maxRetryTimeout) { @Override public void download( List urls, + Map> headers, Credentials credentials, Optional checksum, String canonicalId, @@ -93,7 +96,7 @@ public void download( for (URL url : urls) { SEMAPHORE.acquire(); - try (HttpStream payload = multiplexer.connect(url, checksum, credentials, type); + try (HttpStream payload = multiplexer.connect(url, checksum, headers, credentials, type); OutputStream out = destination.getOutputStream()) { try { ByteStreams.copy(payload, out); @@ -152,7 +155,7 @@ public byte[] downloadAndReadOneUrl( ByteArrayOutputStream out = new ByteArrayOutputStream(); SEMAPHORE.acquire(); try (HttpStream payload = - multiplexer.connect(url, Optional.empty(), credentials, Optional.empty())) { + multiplexer.connect(url, Optional.empty(), ImmutableMap.of(), credentials, Optional.empty())) { ByteStreams.copy(payload, out); } catch (SocketTimeoutException e) { // SocketTimeoutExceptions are InterruptedIOExceptions; however they do not signify diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index cd5f6d3a644c76..8badaacea4d380 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -223,44 +223,18 @@ private static ImmutableMap>> getAuthHeaders( return res; } - private static ImmutableMap>> getHeaders( - Map>> auth) throws RepositoryFunctionException, EvalException { - ImmutableMap.Builder>> headers = new ImmutableMap.Builder<>(); - for (Map.Entry>> entry : auth.entrySet()) { - try { - URL url = new URL(entry.getKey()); - Map> headerMap = entry.getValue(); - headers.put(url.toURI(), headerMap); - } catch (MalformedURLException e) { - throw new RepositoryFunctionException(e, Transience.PERSISTENT); - } catch (URISyntaxException e) { - throw new EvalException(e); - } - } - return headers.buildOrThrow(); - } - - private static ImmutableMap>> getHeaderContents(Dict x, String what) + private static ImmutableMap> getHeaderContents(Dict x, String what) throws EvalException { // Dict.cast returns Dict. @SuppressWarnings({"unchecked", "rawtypes"}) - Map> urlHeaderMapUnchecked = (Map) Dict.cast(x, String.class, Dict.class, what); - - ImmutableMap.Builder>> urlHeadersMap = new ImmutableMap.Builder<>(); - - for (Map.Entry> urlHeaderEntry : urlHeaderMapUnchecked.entrySet()) { - - ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); - Dict headersUnchecked = Dict.cast(urlHeaderEntry.getValue(), String.class, List.class, "header entry"); - - for (Map.Entry headerEntry : headersUnchecked.entrySet()) { - List headerValue = headerEntry.getValue().stream().map(r -> r.toString()).toList(); - headers.put(headerEntry.getKey(), headerValue); - } + Dict headersUnchecked = (Dict) Dict.cast(x, String.class, List.class, what); + ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); - urlHeadersMap.put(urlHeaderEntry.getKey(), headers.buildOrThrow()); + for (Map.Entry headerEntry : headersUnchecked.entrySet()) { + List headerValue = headerEntry.getValue().stream().map(r -> r.toString()).toList(); + headers.put(headerEntry.getKey(), headerValue); } - return urlHeadersMap.buildOrThrow(); + return headers.buildOrThrow(); } private static ImmutableList checkAllUrls(Iterable urlList) throws EvalException { @@ -488,7 +462,7 @@ public StructImpl download( Boolean executable, Boolean allowFail, String canonicalId, - Dict authUnchecked, // expected + Dict authUnchecked, // > expected Dict headersUnchecked, // expected String integrity, StarlarkThread thread) @@ -496,12 +470,7 @@ public StructImpl download( ImmutableMap>> authHeaders = getAuthHeaders(getAuthContents(authUnchecked, "auth")); - ImmutableMap>> headers = getHeaders(getHeaderContents(headersUnchecked, "headers")); - - ImmutableMap>> allHeaders = new ImmutableMap.Builder>>() - .putAll(authHeaders) - .putAll(headers) - .buildOrThrow(); + ImmutableMap> headers = getHeaderContents(headersUnchecked, "headers"); ImmutableList urls = getUrls( @@ -536,7 +505,8 @@ public StructImpl download( downloadedPath = downloadManager.download( urls, - allHeaders, + headers, + authHeaders, checksum, canonicalId, Optional.empty(), @@ -696,12 +666,7 @@ public StructImpl downloadAndExtract( ImmutableMap>> authHeaders = getAuthHeaders(getAuthContents(authUnchecked, "auth")); - ImmutableMap>> headers = getHeaders(getHeaderContents(headersUnchecked, "headers")); - - ImmutableMap>> allHeaders = new ImmutableMap.Builder>>() - .putAll(authHeaders) - .putAll(headers) - .buildOrThrow(); + ImmutableMap> headers = getHeaderContents(headersUnchecked, "headers"); ImmutableList urls = getUrls( @@ -750,7 +715,8 @@ public StructImpl downloadAndExtract( downloadedPath = downloadManager.download( urls, - allHeaders, + headers, + authHeaders, checksum, canonicalId, Optional.of(type), diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 1a201e25cc4bd2..6ce50222e9cb89 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -110,6 +110,7 @@ public void close() { @Override public void download( List urls, + Map> headers, Credentials credentials, Optional checksum, String canonicalId, @@ -154,7 +155,7 @@ public void download( eventHandler.handle( Event.warn("Remote Cache: " + Utils.grpcAwareErrorMessage(e, verboseFailures))); fallbackDownloader.download( - urls, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); + urls, headers, credentials, checksum, canonicalId, destination, eventHandler, clientEnv, type); } } From e40f4ea8ee48c887dc1f305850e57bdc20e6cc4a Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 30 Oct 2023 10:15:35 -0700 Subject: [PATCH 03/16] adress nits --- .../repository/downloader/HttpConnectorMultiplexer.java | 7 ++++--- .../lib/bazel/repository/downloader/HttpDownloader.java | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index 72ef98afe59575..ece993d2dc31b9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -75,7 +75,7 @@ final class HttpConnectorMultiplexer { } public HttpStream connect(URL url, Optional checksum) throws IOException { - return connect(url, checksum, Map.of(), StaticCredentials.EMPTY, Optional.empty()); + return connect(url, checksum, ImmutableMap.of(), StaticCredentials.EMPTY, Optional.empty()); } /** @@ -102,12 +102,13 @@ public HttpStream connect( if (Thread.interrupted()) { throw new InterruptedIOException(); } - Map> baseHeaders = new HashMap<>(headers); + ImmutableMap.Builder> baseHeaders = new ImmutableMap.Builder(); + baseHeaders.putAll(headers); // REQUEST_HEADERS should not be overriable by user provided headers baseHeaders.putAll(REQUEST_HEADERS); Function>> headerFunction = - getHeaderFunction(baseHeaders, credentials); + getHeaderFunction(baseHeaders.buildOrThrow(), credentials); URLConnection connection = connector.connect(url, headerFunction); return httpStreamFactory.create( connection, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java index 6464aad4caeffd..e960bdd128e094 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloader.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.bazel.repository.downloader; import com.google.auth.Credentials; -import com.google.common.base.Optional; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; From fa54b76964d53cb36b50b325592093b6fa3b4066 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 30 Oct 2023 10:40:35 -0700 Subject: [PATCH 04/16] remove .toList --- .../bazel/repository/starlark/StarlarkBaseExternalContext.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 8badaacea4d380..7036117cee7442 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -77,6 +77,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; import javax.annotation.Nullable; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; @@ -231,7 +232,7 @@ private static ImmutableMap> getHeaderContents(Dict x ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); for (Map.Entry headerEntry : headersUnchecked.entrySet()) { - List headerValue = headerEntry.getValue().stream().map(r -> r.toString()).toList(); + List headerValue = (List) headerEntry.getValue().stream().map(r -> r.toString()).collect(Collectors.toList()); headers.put(headerEntry.getKey(), headerValue); } return headers.buildOrThrow(); From 9637472e95f08fe6e5b6666824762d96c9157dff Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 30 Oct 2023 11:14:05 -0700 Subject: [PATCH 05/16] fix tests --- .../repository/downloader/HttpDownloaderTest.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java index 2451af6027aabf..0e59d412abb975 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpDownloaderTest.java @@ -117,6 +117,7 @@ public void downloadFrom1UrlOk() throws IOException, InterruptedException { Collections.singletonList( new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))), Collections.emptyMap(), + Collections.emptyMap(), Optional.empty(), "testCanonicalId", Optional.empty(), @@ -181,6 +182,7 @@ public void downloadFrom2UrlsFirstOk() throws IOException, InterruptedException downloadManager.download( urls, Collections.emptyMap(), + Collections.emptyMap(), Optional.empty(), "testCanonicalId", Optional.empty(), @@ -248,6 +250,7 @@ public void downloadFrom2UrlsFirstSocketTimeoutOnBodyReadSecondOk() downloadManager.download( urls, Collections.emptyMap(), + Collections.emptyMap(), Optional.empty(), "testCanonicalId", Optional.empty(), @@ -317,6 +320,7 @@ public void downloadFrom2UrlsBothSocketTimeoutDuringBodyRead() downloadManager.download( urls, Collections.emptyMap(), + Collections.emptyMap(), Optional.empty(), "testCanonicalId", Optional.empty(), @@ -371,6 +375,7 @@ public void downloadOneUrl_ok() throws IOException, InterruptedException { httpDownloader.download( Collections.singletonList( new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))), + Collections.emptyMap(), StaticCredentials.EMPTY, Optional.empty(), "testCanonicalId", @@ -410,6 +415,7 @@ public void downloadOneUrl_notFound() throws IOException, InterruptedException { httpDownloader.download( Collections.singletonList( new URL(String.format("http://localhost:%d/foo", server.getLocalPort()))), + Collections.emptyMap(), StaticCredentials.EMPTY, Optional.empty(), "testCanonicalId", @@ -470,6 +476,7 @@ public void downloadTwoUrls_firstNotFoundAndSecondOk() throws IOException, Inter Path destination = fs.getPath(workingDir.newFile().getAbsolutePath()); httpDownloader.download( urls, + Collections.emptyMap(), StaticCredentials.EMPTY, Optional.empty(), "testCanonicalId", @@ -564,13 +571,14 @@ public void download_contentLengthMismatch_propagateErrorIfNotRetry() throws Exc throw new ContentLengthMismatchException(0, data.length); }) .when(downloader) - .download(any(), any(), any(), any(), any(), any(), any(), any()); + .download(any(), any(), any(), any(), any(), any(), any(), any(), any()); assertThrows( ContentLengthMismatchException.class, () -> downloadManager.download( ImmutableList.of(new URL("http://localhost")), + Collections.emptyMap(), ImmutableMap.of(), Optional.empty(), "testCanonicalId", @@ -597,7 +605,7 @@ public void download_contentLengthMismatch_retries() throws Exception { if (times.getAndIncrement() < 3) { throw new ContentLengthMismatchException(0, data.length); } - Path output = invocationOnMock.getArgument(4, Path.class); + Path output = invocationOnMock.getArgument(5, Path.class); try (OutputStream outputStream = output.getOutputStream()) { ByteStreams.copy(new ByteArrayInputStream(data), outputStream); } @@ -605,12 +613,13 @@ public void download_contentLengthMismatch_retries() throws Exception { return null; }) .when(downloader) - .download(any(), any(), any(), any(), any(), any(), any(), any()); + .download(any(), any(), any(), any(), any(), any(), any(), any(), any()); Path result = downloadManager.download( ImmutableList.of(new URL("http://localhost")), ImmutableMap.of(), + ImmutableMap.of(), Optional.empty(), "testCanonicalId", Optional.empty(), From f12b138ee8763b4c514ec70965c71337025fa912 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Mon, 30 Oct 2023 11:30:16 -0700 Subject: [PATCH 06/16] fix tests --- .../lib/remote/downloader/GrpcRemoteDownloaderTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index 45fee7df1a7b30..5068db0e15807f 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -178,6 +178,7 @@ private static byte[] downloadBlob( final Path destination = scratch.resolve("output file path"); downloader.download( urls, + ImmutableMap.of(), StaticCredentials.EMPTY, checksum, canonicalId, @@ -240,13 +241,13 @@ public void fetchBlob( invocation -> { List urls = invocation.getArgument(0); if (urls.equals(ImmutableList.of(new URL("http://example.com/content.txt")))) { - Path output = invocation.getArgument(4); + Path output = invocation.getArgument(5); FileSystemUtils.writeContent(output, content); } return null; }) .when(fallbackDownloader) - .download(any(), any(), any(), any(), any(), any(), any(), any()); + .download(any(), any(), any(), any(), any(), any(), any(), any(), any()); final GrpcRemoteDownloader downloader = newDownloader(cacheClient, fallbackDownloader); final byte[] downloaded = From 2eecc543bfd909e198d9aefd546c7f4b0bb8a4ab Mon Sep 17 00:00:00 2001 From: Sahin Yort Date: Thu, 23 Nov 2023 13:20:38 -0800 Subject: [PATCH 07/16] Update src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Co-authored-by: Fabian Meumertzheim --- .../bazel/repository/downloader/HttpConnectorMultiplexer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index ece993d2dc31b9..970256aad75721 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -104,7 +104,7 @@ public HttpStream connect( } ImmutableMap.Builder> baseHeaders = new ImmutableMap.Builder(); baseHeaders.putAll(headers); - // REQUEST_HEADERS should not be overriable by user provided headers + // REQUEST_HEADERS should not be overridable by user provided headers baseHeaders.putAll(REQUEST_HEADERS); Function>> headerFunction = From 8a52342f77ce5db3f07ace0321d54a9f1dcd927a Mon Sep 17 00:00:00 2001 From: Sahin Yort Date: Thu, 23 Nov 2023 13:20:44 -0800 Subject: [PATCH 08/16] Update src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java Co-authored-by: Fabian Meumertzheim --- .../bazel/repository/downloader/HttpConnectorMultiplexer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index 970256aad75721..5f16de9a4de13b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -108,7 +108,7 @@ public HttpStream connect( baseHeaders.putAll(REQUEST_HEADERS); Function>> headerFunction = - getHeaderFunction(baseHeaders.buildOrThrow(), credentials); + getHeaderFunction(baseHeaders.buildKeepingLast(), credentials); URLConnection connection = connector.connect(url, headerFunction); return httpStreamFactory.create( connection, From d07484bc8751f0875a954c443f4f8b6dff26147b Mon Sep 17 00:00:00 2001 From: thesayyn Date: Thu, 23 Nov 2023 22:01:03 -0800 Subject: [PATCH 09/16] add tests --- .../starlark/StarlarkBaseExternalContext.java | 10 +- src/test/shell/bazel/remote_helpers.sh | 19 ++ .../shell/bazel/starlark_repository_test.sh | 163 ++++++++++++++++++ src/test/shell/bazel/testing_server.py | 17 +- 4 files changed, 203 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 7036117cee7442..855d0db7dc5486 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -228,11 +228,11 @@ private static ImmutableMap> getHeaderContents(Dict x throws EvalException { // Dict.cast returns Dict. @SuppressWarnings({"unchecked", "rawtypes"}) - Dict headersUnchecked = (Dict) Dict.cast(x, String.class, List.class, what); + Dict headersUnchecked = (Dict) Dict.cast(x, String.class, Sequence.class, what); ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); - for (Map.Entry headerEntry : headersUnchecked.entrySet()) { - List headerValue = (List) headerEntry.getValue().stream().map(r -> r.toString()).collect(Collectors.toList()); + for (Map.Entry headerEntry : headersUnchecked.entrySet()) { + List headerValue = Sequence.cast(headerEntry.getValue(), String.class, "header values").getImmutableList(); headers.put(headerEntry.getKey(), headerValue); } return headers.buildOrThrow(); @@ -280,9 +280,9 @@ private static ImmutableList getUrls( new IOException("Unsupported protocol: " + url.getProtocol()), Transience.PERSISTENT); } if (!checksumGiven) { - if (!Ascii.equalsIgnoreCase("http", url.getProtocol())) { + //if (!Ascii.equalsIgnoreCase("http", url.getProtocol())) { urls.add(url); - } + // } } else { urls.add(url); } diff --git a/src/test/shell/bazel/remote_helpers.sh b/src/test/shell/bazel/remote_helpers.sh index 6a66398d3eabd9..e70be5077b5cce 100755 --- a/src/test/shell/bazel/remote_helpers.sh +++ b/src/test/shell/bazel/remote_helpers.sh @@ -179,6 +179,25 @@ function serve_timeout() { cd - } +# Serves a HTTP 200 Ok response with headers dumped into the body +function serve_file_header_dump() { + file_name=served_file.$$ + cat $1 > "${TEST_TMPDIR}/$file_name" + nc_log="${TEST_TMPDIR}/nc.log" + rm -f $nc_log + touch $nc_log + cd "${TEST_TMPDIR}" + port_file=server-port.$$ + rm -f $port_file + python3 $python_server always $file_name --dump_headers ${2:-"headers.json"} > $port_file & + nc_pid=$! + while ! grep started $port_file; do sleep 1; done + nc_port=$(head -n 1 $port_file) + fileserver_port=$nc_port + wait_for_server_startup + cd - +} + # Waits for the SimpleHTTPServer to actually start up before the test is run. # Otherwise the entire test can run before the server starts listening for # connections, which causes flakes. diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 9c7fc508539657..45a7ad8f830819 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2408,4 +2408,167 @@ EOF || fail "Expected build to succeed" } + +function test_cred_helper_overrides_starlark_headers() { + if "$is_windows"; then + # Skip on Windows: credential helper is a Python script. + return + fi + + setup_credential_helper + + echo "test" > test.txt + sha256="$(sha256sum test.txt | head -c 64)" + serve_file_header_dump test.txt credhelper_headers.json + + cat > WORKSPACE < test.bzl < test.txt + sha256="$(sha256sum test.txt | head -c 64)" + serve_file_header_dump test.txt netrc_headers.json + + cat > WORKSPACE < .netrc < test.bzl < test.txt + sha256="$(sha256sum test.txt | head -c 64)" + serve_file_header_dump test.txt default_headers.json + + cat > WORKSPACE < test.bzl < test.txt + sha256="$(sha256sum test.txt | head -c 64)" + serve_file_header_dump test.txt invalidheaders.json + + cat > WORKSPACE < test.bzl <& $TEST_log && fail "expected bazel to fail" + expect_log "Error in download: got dict for 'headers', want dict" +} + run_suite "local repository tests" diff --git a/src/test/shell/bazel/testing_server.py b/src/test/shell/bazel/testing_server.py index 00c714792d2d95..8c3b6b2e89e3d9 100644 --- a/src/test/shell/bazel/testing_server.py +++ b/src/test/shell/bazel/testing_server.py @@ -17,6 +17,7 @@ # pylint: disable=g-import-not-at-top,g-importing-member import argparse import base64 +import json try: from http.server import BaseHTTPRequestHandler except ImportError: @@ -44,11 +45,16 @@ class Handler(BaseHTTPRequestHandler): auth = False not_found = False simulate_timeout = False + dump_headers = None filename = None redirect = None valid_headers = [ b'Basic ' + base64.b64encode('foo:bar'.encode('ascii')), b'Bearer TOKEN' ] + unstable_headers = [ + "Host", + "User-Agent" + ] def do_HEAD(self): # pylint: disable=invalid-name self.send_response(200) @@ -66,6 +72,11 @@ def do_GET(self): # pylint: disable=invalid-name # Needed for Unix domain connections as the response functions # fail without this being set. self.client_address = 'localhost' + + if self.dump_headers: + headers = filter(lambda hv: hv[0] not in self.unstable_headers, self.headers.items()) + with open(self.dump_headers, 'w') as f: + f.write(json.dumps(dict(headers))) if self.simulate_timeout: while True: @@ -75,7 +86,7 @@ def do_GET(self): # pylint: disable=invalid-name self.send_response(404) self.end_headers() return - + if self.redirect is not None: self.send_response(301) self.send_header('Location', self.redirect) @@ -110,6 +121,8 @@ def serve_file(self): def main(argv): parser = argparse.ArgumentParser() parser.add_argument('--unix_socket', action='store') + parser.add_argument('--dump_headers', action='store') + parser.add_argument('mode', type=str, nargs='?') parser.add_argument('target', type=str, nargs='?') args = parser.parse_args(argv) @@ -127,6 +140,8 @@ def main(argv): Handler.auth = True if args.target: Handler.filename = args.target + + Handler.dump_headers = args.dump_headers httpd = None if args.unix_socket: From 8b6cbba4d8b80b5b9ef61cd37cde0d95f513bde7 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Fri, 24 Nov 2023 08:00:51 -0800 Subject: [PATCH 10/16] revert --- .../repository/starlark/StarlarkBaseExternalContext.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 36f84c668104df..f61a8ba8685f65 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -280,9 +280,9 @@ private static ImmutableList getUrls( new IOException("Unsupported protocol: " + url.getProtocol()), Transience.PERSISTENT); } if (!checksumGiven) { - //if (!Ascii.equalsIgnoreCase("http", url.getProtocol())) { + if (!Ascii.equalsIgnoreCase("http", url.getProtocol())) { urls.add(url); - // } + } } else { urls.add(url); } From 532e4d3bc34d304f3effdd3f106ae758cae46fab Mon Sep 17 00:00:00 2001 From: thesayyn Date: Fri, 24 Nov 2023 11:03:26 -0800 Subject: [PATCH 11/16] fix presubmit --- src/test/shell/bazel/remote_helpers.sh | 2 +- .../shell/bazel/starlark_repository_test.sh | 80 ++++++++----------- 2 files changed, 33 insertions(+), 49 deletions(-) diff --git a/src/test/shell/bazel/remote_helpers.sh b/src/test/shell/bazel/remote_helpers.sh index e70be5077b5cce..ab2cfd0c8a819d 100755 --- a/src/test/shell/bazel/remote_helpers.sh +++ b/src/test/shell/bazel/remote_helpers.sh @@ -179,7 +179,7 @@ function serve_timeout() { cd - } -# Serves a HTTP 200 Ok response with headers dumped into the body +# Serves a HTTP 200 Ok response with headers dumped into the file function serve_file_header_dump() { file_name=served_file.$$ cat $1 > "${TEST_TMPDIR}/$file_name" diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 5fb03385fc35bd..10383305b3eb28 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2417,22 +2417,18 @@ function test_cred_helper_overrides_starlark_headers() { setup_credential_helper - echo "test" > test.txt - sha256="$(sha256sum test.txt | head -c 64)" - serve_file_header_dump test.txt credhelper_headers.json + filename="cred_helper_starlark.txt" + echo $filename > $filename + sha256="$(sha256sum $filename | head -c 64)" + serve_file_header_dump $filename credhelper_headers.json - cat > WORKSPACE < test.bzl < test.txt - sha256="$(sha256sum test.txt | head -c 64)" - serve_file_header_dump test.txt netrc_headers.json + filename="netrc_headers.txt" + echo $filename > $filename + sha256="$(sha256sum $filename | head -c 64)" + serve_file_header_dump $filename netrc_headers.json - cat > WORKSPACE < .netrc < test.txt - sha256="$(sha256sum test.txt | head -c 64)" - serve_file_header_dump test.txt default_headers.json - cat > WORKSPACE < $filename + sha256="$(sha256sum $filename | head -c 64)" + serve_file_header_dump $filename default_headers.json - touch BUILD.bazel + setup_starlark_repository cat > test.bzl < test.txt - sha256="$(sha256sum test.txt | head -c 64)" - serve_file_header_dump test.txt invalidheaders.json - cat > WORKSPACE < $filename + sha256="$(sha256sum $filename | head -c 64)" + serve_file_header_dump $filename invalid_headers.json - touch BUILD.bazel + setup_starlark_repository cat > test.bzl <& $TEST_log && fail "expected bazel to fail" + bazel build @foo//:all >& $TEST_log && fail "expected bazel to fail" || : expect_log "Error in download: got dict for 'headers', want dict" } From ef6b3c10a51d809eccf168effb83e5a82ee2df01 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Tue, 28 Nov 2023 08:05:08 -0800 Subject: [PATCH 12/16] --noenable_bzlmod --- src/test/shell/bazel/starlark_repository_test.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 10383305b3eb28..32d7f753419dd0 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2354,8 +2354,8 @@ genrule( cmd = "cp $< $@", ) EOF - - bazel build --repository_disable_download //:it || fail "Failed to build" + # for some reason --repository_disable_download fails with bzlmod trying to download @platforms. + bazel build --repository_disable_download --noenable_bzlmod //:it || fail "Failed to build" } function test_no_restarts_fetching_with_worker_thread() { From a31e5adb785d819a54f3b8698a93d65e7a3292bf Mon Sep 17 00:00:00 2001 From: thesayyn Date: Tue, 28 Nov 2023 11:46:18 -0800 Subject: [PATCH 13/16] doc --- .../repository/starlark/StarlarkBaseExternalContext.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index f61a8ba8685f65..70b8408c3c95a2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -443,7 +443,7 @@ private StructImpl calculateDownloadResult(Optional checksum, Path dow name = "headers", defaultValue = "{}", named = true, - doc = "An optional dict specifying http headers for some of the URLs."), + doc = "An optional dict specifying http headers for all URLs."), @Param( name = "integrity", defaultValue = "''", @@ -623,7 +623,7 @@ public StructImpl download( name = "headers", defaultValue = "{}", named = true, - doc = "An optional dict specifying http headers for some of the URLs."), + doc = "An optional dict specifying http headers for all URLs."), @Param( name = "integrity", defaultValue = "''", From 47ef7f0d24b04c684dd369897f8464f139cfeb9b Mon Sep 17 00:00:00 2001 From: thesayyn Date: Thu, 30 Nov 2023 13:09:13 -0800 Subject: [PATCH 14/16] nits --- .../repository/starlark/StarlarkBaseExternalContext.java | 6 +++--- src/test/shell/bazel/remote_helpers.sh | 3 +++ src/test/shell/bazel/starlark_repository_test.sh | 3 +-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 70b8408c3c95a2..cc2bf12a22eedc 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -463,8 +463,8 @@ public StructImpl download( Boolean executable, Boolean allowFail, String canonicalId, - Dict authUnchecked, // > expected - Dict headersUnchecked, // expected + Dict authUnchecked, // expected + Dict headersUnchecked, // | String> expected String integrity, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { @@ -656,7 +656,7 @@ public StructImpl downloadAndExtract( Boolean allowFail, String canonicalId, Dict authUnchecked, // expected - Dict headersUnchecked, // expected + Dict headersUnchecked, // | String> expected String integrity, Dict renameFiles, // expected StarlarkThread thread) diff --git a/src/test/shell/bazel/remote_helpers.sh b/src/test/shell/bazel/remote_helpers.sh index ab2cfd0c8a819d..66b3899794bc9d 100755 --- a/src/test/shell/bazel/remote_helpers.sh +++ b/src/test/shell/bazel/remote_helpers.sh @@ -180,6 +180,9 @@ function serve_timeout() { } # Serves a HTTP 200 Ok response with headers dumped into the file +# Args: +# $1: required; path to the file +# $2: optional; path to the file where headers will be written to. function serve_file_header_dump() { file_name=served_file.$$ cat $1 > "${TEST_TMPDIR}/$file_name" diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 32d7f753419dd0..932d7c987a9c92 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2495,7 +2495,7 @@ EOF } -function test_starlark_headers_should_override_default_headers() { +function test_starlark_headers_override_default_headers() { filename="default_headers.txt" echo $filename > $filename @@ -2515,7 +2515,6 @@ def _impl(repository_ctx): "Accept": ["application/vnd.oci.image.index.v1+json, application/vnd.oci.image.manifest.v1+json"], } ) - print("here") repo = repository_rule(implementation=_impl) EOF From c87128f9c72986d83b1b13881f3b7ffc59bff3e9 Mon Sep 17 00:00:00 2001 From: thesayyn Date: Fri, 1 Dec 2023 14:05:22 -0800 Subject: [PATCH 15/16] support syntax sugar string in values --- .../starlark/StarlarkBaseExternalContext.java | 26 +++++++++++----- .../shell/bazel/starlark_repository_test.sh | 31 ++++++++++++++++++- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index cc2bf12a22eedc..2a96677a5a0a5e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -225,19 +225,29 @@ private static ImmutableMap>> getAuthHeaders( } private static ImmutableMap> getHeaderContents(Dict x, String what) - throws EvalException { + throws EvalException { // Dict.cast returns Dict. - @SuppressWarnings({"unchecked", "rawtypes"}) - Dict headersUnchecked = (Dict) Dict.cast(x, String.class, Sequence.class, what); + @SuppressWarnings({ "unchecked", "rawtypes" }) + Dict headersUnchecked = (Dict) Dict.cast(x, String.class, Object.class, what); ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); - for (Map.Entry headerEntry : headersUnchecked.entrySet()) { - List headerValue = Sequence.cast(headerEntry.getValue(), String.class, "header values").getImmutableList(); - headers.put(headerEntry.getKey(), headerValue); + for (Map.Entry entry : headersUnchecked.entrySet()) { + List headerValue; + Object valueUnchecked = entry.getValue(); + if (valueUnchecked instanceof Sequence) { + headerValue = Sequence.cast(valueUnchecked, String.class, "header values").getImmutableList(); + } else if (valueUnchecked instanceof String) { + headerValue = List.of(valueUnchecked.toString()); + } else { + throw new EvalException( + String.format("Trying to build %s, the value in the headers dict" + + " must be a string or string sequence.", what)); + } + headers.put(entry.getKey(), headerValue); } return headers.buildOrThrow(); -} - + } + private static ImmutableList checkAllUrls(Iterable urlList) throws EvalException { ImmutableList.Builder result = ImmutableList.builder(); diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 932d7c987a9c92..2712d0e75313fa 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2551,7 +2551,36 @@ repo = repository_rule(implementation=_impl) EOF bazel build @foo//:all >& $TEST_log && fail "expected bazel to fail" || : - expect_log "Error in download: got dict for 'headers', want dict" + expect_log "Trying to build headers, the value in the headers dict must be a string or string sequence." +} + +function test_string_starlark_headers() { + + filename="string_headers.txt" + echo $filename > $filename + sha256="$(sha256sum $filename | head -c 64)" + serve_file_header_dump $filename string_headers.json + + setup_starlark_repository + + cat > test.bzl < Date: Wed, 6 Dec 2023 14:44:53 -0800 Subject: [PATCH 16/16] nits --- .../repository/starlark/StarlarkBaseExternalContext.java | 7 ++----- src/test/shell/bazel/starlark_repository_test.sh | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 2a96677a5a0a5e..d012d085fda859 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -226,9 +226,7 @@ private static ImmutableMap>> getAuthHeaders( private static ImmutableMap> getHeaderContents(Dict x, String what) throws EvalException { - // Dict.cast returns Dict. - @SuppressWarnings({ "unchecked", "rawtypes" }) - Dict headersUnchecked = (Dict) Dict.cast(x, String.class, Object.class, what); + Dict headersUnchecked = (Dict) Dict.cast(x, String.class, Object.class, what); ImmutableMap.Builder> headers = new ImmutableMap.Builder<>(); for (Map.Entry entry : headersUnchecked.entrySet()) { @@ -240,8 +238,7 @@ private static ImmutableMap> getHeaderContents(Dict x headerValue = List.of(valueUnchecked.toString()); } else { throw new EvalException( - String.format("Trying to build %s, the value in the headers dict" - + " must be a string or string sequence.", what)); + String.format("%s argument must be a dict whose keys are string and whose values are either string or sequence of string", what)); } headers.put(entry.getKey(), headerValue); } diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 2712d0e75313fa..fcf705ee5f54ca 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2551,7 +2551,7 @@ repo = repository_rule(implementation=_impl) EOF bazel build @foo//:all >& $TEST_log && fail "expected bazel to fail" || : - expect_log "Trying to build headers, the value in the headers dict must be a string or string sequence." + expect_log "headers argument must be a dict whose keys are string and whose values are either string or sequence of string" } function test_string_starlark_headers() {