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

Fix Bazel incompatible issues. #648

Merged
merged 9 commits into from
Jan 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ platforms:
# tests/contrib/test_compare_ids_test_* expect 'bazel' on path
test_flags:
- "--action_env=PATH"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
ubuntu1604:
test_targets:
# We are running the skipped targets remotely only.
Expand All @@ -36,15 +39,24 @@ platforms:
# tests/contrib/test_compare_ids_test_* expect 'bazel' on path
test_flags:
- "--action_env=PATH"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
macos:
build_targets:
- "//tests/docker:test_digest_output1"
build_flags:
- "--action_env=PATH"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
test_targets:
- "//tests/docker:test_digest_output1"
test_flags:
- "--action_env=PATH"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
rbe_ubuntu1604:
build_targets:
- "--"
Expand All @@ -53,6 +65,9 @@ platforms:
- "--extra_execution_platforms=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--host_platform=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--platforms=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
test_targets:
- "--"
- "//tests/..."
Expand All @@ -66,3 +81,6 @@ platforms:
- "--extra_execution_platforms=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--host_platform=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--platforms=@bazel_toolchains//configs/ubuntu16_04_clang/1.1:nosla_xenial_docker"
- "--all_incompatible_changes"
# TODO(xingao): Remove after https://github.com/bazelbuild/rules_scala/issues/644 is addressed.
- "--incompatible_disallow_legacy_javainfo=false"
26 changes: 15 additions & 11 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -160,19 +160,23 @@ load(

_java_image_repos()

load("@bazel_tools//tools/build_defs/repo:jvm.bzl", "jvm_maven_import_external")

# For our java_image test.
maven_jar(
jvm_maven_import_external(
name = "com_google_guava_guava",
artifact = "com.google.guava:guava:18.0",
sha1 = "cce0823396aa693798f8882e64213b1772032b09",
artifact_sha256 = "d664fbfc03d2e5ce9cab2a44fb01f1d0bf9dfebeccc1a473b1f9ea31f79f6f99",
licenses = ["notice"], # Apache 2.0
server_urls = ["http://central.maven.org/maven2"],
)

# For our scala_image test.
http_archive(
name = "io_bazel_rules_scala",
sha256 = "83d40e0bc7377e77fa0d32af6c4b276374b4efbcb3120c437e425947cdb3ce38",
strip_prefix = "rules_scala-5130b97524684beceba729b9dab1528e2a90cdfb",
urls = ["https://github.com/bazelbuild/rules_scala/archive/5130b97524684beceba729b9dab1528e2a90cdfb.tar.gz"],
sha256 = "902e30b931ded41905641895b90c41727e01a732aba67dfda604b764c1e1e494",
strip_prefix = "rules_scala-1354d935a74395b3f0870dd90a04e0376fe22587",
urls = ["https://github.com/bazelbuild/rules_scala/archive/1354d935a74395b3f0870dd90a04e0376fe22587.tar.gz"],
)

load("@io_bazel_rules_scala//scala:scala.bzl", "scala_repositories")
Expand Down Expand Up @@ -219,9 +223,9 @@ _go_image_repos()
# For our rust_image test
http_archive(
name = "io_bazel_rules_rust",
sha256 = "500d06096a44ff6d77256635dbe6ab61b23c2be626e2acb08a4c060092e711d0",
strip_prefix = "rules_rust-db81b42d98e1232e001e26a50c37f2097d61a207",
urls = ["https://github.com/bazelbuild/rules_rust/archive/db81b42d98e1232e001e26a50c37f2097d61a207.tar.gz"],
sha256 = "ed0c81084bcc2bdcc98cfe56f384b20856840825f5e413e2b71809b61809fc87",
strip_prefix = "rules_rust-f32695dcd02d9a19e42b9eb7f29a24a8ceb2b858",
urls = ["https://github.com/bazelbuild/rules_rust/archive/f32695dcd02d9a19e42b9eb7f29a24a8ceb2b858.tar.gz"],
)

load("@io_bazel_rules_rust//rust:repositories.bzl", "rust_repositories")
Expand All @@ -237,9 +241,9 @@ bazel_version(name = "bazel_version")
# For our d_image test
http_archive(
name = "io_bazel_rules_d",
sha256 = "527908e02d7bccf5a4eb89b690b003247eb6c57d69cc3234977c034d27c59d6e",
strip_prefix = "rules_d-0400b9b054013274cee2ed15679da19e1fc94e07",
urls = ["https://github.com/bazelbuild/rules_d/archive/0400b9b054013274cee2ed15679da19e1fc94e07.tar.gz"],
sha256 = "873022774f2f31ab57e7ff36b3f39c60fd4209952bfcc6902924b7942fa2973d",
strip_prefix = "rules_d-2d38613073f3eb138aee0acbcb395ebada2f8ebf",
urls = ["https://github.com/bazelbuild/rules_d/archive/2d38613073f3eb138aee0acbcb395ebada2f8ebf.tar.gz"],
)

load("@io_bazel_rules_d//d:d.bzl", "d_repositories")
Expand Down
6 changes: 3 additions & 3 deletions container/container.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,9 @@ py_library(
if "subpar" not in excludes:
http_archive(
name = "subpar",
sha256 = "ee65e35c1cd9a723fb4d501e8055e10b34a27a0a557d10312af7b83d8e0101f5",
strip_prefix = "subpar-7e12cc130eb8f09c8cb02c3585a91a4043753c56",
urls = ["https://github.com/google/subpar/archive/7e12cc130eb8f09c8cb02c3585a91a4043753c56.tar.gz"],
sha256 = "cf3762b10426a1887d37f127b4c1390785ecb969254096eb714cc1db371f78d6",
strip_prefix = "subpar-a4f9b23bf01bcc7a52d458910af65a90ee991aff",
urls = ["https://github.com/google/subpar/archive/a4f9b23bf01bcc7a52d458910af65a90ee991aff.tar.gz"],
)

if "structure_test_linux" not in excludes:
Expand Down
4 changes: 2 additions & 2 deletions container/image.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ def _assemble_image_digest(ctx, name, image, image_tarball, output_digest):

ctx.actions.run(
outputs = [output_digest],
inputs = [image["config"]] + blobsums + blobs +
([image["legacy"]] if image.get("legacy") else []),
tools = [image["config"]] + blobsums + blobs +
Copy link
Contributor

@ash2k ash2k Mar 7, 2019

Choose a reason for hiding this comment

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

@xingao267 what is the reason for this change? Just trying to understand. I thought tools is for "tools" that are used to perform the action and the inputs are for inputs i.e. files that are processed by the tools. Here all these things are inputs, not files, I think. Same in other files below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I think I made that change because Bazel complains that some of these files were executable, so I put them into tools according to https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#disallow-tools-in-action-inputs. Let me try to change it back to inputs to see if the CI complains about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why they are executable... From that page:

In the rare case that your action requires a tool as input, but does not actually run the tool and therefore does not need its runfiles, the safety check will fail even though the action would have succeeded. In this case, you can bypass the check by adding a (possibly empty) tools argument to your action.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea. I was surprising too. I sent https://github.com/bazelbuild/rules_docker/pull/716/files to changed it back and I think it's fine. Maybe I mistakenly changed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out!

([image["legacy"]] if image.get("legacy") else []),
executable = ctx.executable._digester,
arguments = arguments,
mnemonic = "ImageDigest",
Expand Down
3 changes: 1 addition & 2 deletions container/image_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -708,8 +708,7 @@ def test_java_image(self):
'./app/io_bazel_rules_docker/testdata',
'./app/io_bazel_rules_docker/testdata/libjava_image_library.jar',
'./app/com_google_guava_guava',
'./app/com_google_guava_guava/jar',
'./app/com_google_guava_guava/jar/guava-18.0.jar',
'./app/com_google_guava_guava/guava-18.0.jar',
])

def test_war_image(self):
Expand Down
2 changes: 1 addition & 1 deletion container/layer.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def build_layer(
ctx.actions.run(
executable = build_layer_exec,
arguments = args,
inputs = files + file_map.values() + tars + debs + [manifest_file],
tools = files + file_map.values() + tars + debs + [manifest_file],
outputs = [layer],
use_default_shell_env = True,
mnemonic = "ImageLayer",
Expand Down
4 changes: 2 additions & 2 deletions container/layer_tools.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def _extract_layers(ctx, name, artifact):
"--manifestoutput",
manifest_file.path,
],
inputs = [artifact],
tools = [artifact],
outputs = [config_file, manifest_file],
mnemonic = "ExtractConfig",
)
Expand Down Expand Up @@ -102,7 +102,7 @@ def assemble(ctx, images, output, stamp = False):
ctx.actions.run(
executable = ctx.executable.join_layers,
arguments = args,
inputs = inputs,
tools = inputs,
outputs = [output],
mnemonic = "JoinLayers",
)
Expand Down
2 changes: 1 addition & 1 deletion contrib/compare_ids_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
def _compare_ids_test_impl(ctx):
tar_files = []
for image in ctx.attr.images:
tar_files += list(image.files)
tar_files += image.files.to_list()

if (len(tar_files) == 0):
fail("No images provided for test.")
Expand Down
8 changes: 3 additions & 5 deletions contrib/idd.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Used for finding differences between image targets.
Args:
image1: Image target or image tarball file (from docker save) - first image to compare
image2: Image target or image tarball file (from docker save) - second image to compare
args: (optional) list of strings - arguments to apply to idd.py call
args: (optional) list of strings - arguments to apply to idd.py call
refer to idd.py docs for more info

Ex.
Expand All @@ -56,13 +56,11 @@ idd = rule(
attrs = {
"image1": attr.label(
mandatory = True,
allow_files = container_filetype,
single_file = True,
allow_single_file = container_filetype,
),
"image2": attr.label(
mandatory = True,
allow_files = container_filetype,
single_file = True,
allow_single_file = container_filetype,
),
"_idd_script": attr.label(
default = ":idd",
Expand Down
8 changes: 3 additions & 5 deletions contrib/push-all.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def _impl(ctx):

return struct(runfiles = ctx.runfiles(files = [
ctx.executable._pusher,
] + stamp_inputs + runfiles + list(ctx.attr._pusher.default_runfiles.files)))
] + stamp_inputs + runfiles + ctx.attr._pusher.default_runfiles.files.to_list()))

container_push = rule(
attrs = {
Expand All @@ -110,13 +110,11 @@ container_push = rule(
),
"_all_tpl": attr.label(
default = Label("//contrib:push-all.sh.tpl"),
single_file = True,
allow_files = True,
allow_single_file = True,
),
"_tag_tpl": attr.label(
default = Label("//container:push-tag.sh.tpl"),
single_file = True,
allow_files = True,
allow_single_file = True,
),
"_pusher": attr.label(
default = Label("@containerregistry//:pusher"),
Expand Down
6 changes: 2 additions & 4 deletions contrib/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ _container_test = rule(
),
"image_tar": attr.label(
doc = "When using the tar driver, label of the container image tarball",
allow_files = [".tar"],
single_file = True,
allow_single_file = [".tar"],
),
"loaded_name": attr.string(
doc = "When using the docker driver, the name:tag of the image when loaded into the docker daemon",
Expand Down Expand Up @@ -105,8 +104,7 @@ _container_test = rule(
),
"_structure_test_tpl": attr.label(
default = Label("//contrib:structure-test.sh.tpl"),
allow_files = True,
single_file = True,
allow_single_file = True,
),
},
executable = True,
Expand Down
40 changes: 18 additions & 22 deletions java/image.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ The signature of java_image is compatible with java_binary.
The signature of war_image is compatible with java_library.
"""

load("@bazel_tools//tools/build_defs/repo:jvm.bzl", "jvm_maven_import_external")
load(
"//container:container.bzl",
"container_pull",
Expand Down Expand Up @@ -75,9 +76,12 @@ def repositories():
digest = _JETTY_DIGESTS["debug"],
)
if "javax_servlet_api" not in excludes:
native.maven_jar(
jvm_maven_import_external(
name = "javax_servlet_api",
artifact = "javax.servlet:javax.servlet-api:3.0.1",
artifact_sha256 = "377d8bde87ac6bc7f83f27df8e02456d5870bb78c832dac656ceacc28b016e56",
server_urls = ["http://central.maven.org/maven2"],
licenses = ["notice"], # Apache 2.0
)

DEFAULT_JAVA_BASE = select({
Expand All @@ -98,15 +102,15 @@ def java_files(f):
files = []
if java_common.provider in f:
java_provider = f[java_common.provider]
files += list(java_provider.transitive_runtime_jars)
files += java_provider.transitive_runtime_jars.to_list()
if hasattr(f, "files"): # a jar file
files += list(f.files)
files += f.files.to_list()
return files
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to have java_files and java_files_with_data return depsets, because:

  • most call sites convert the list to a depset
  • you can avoid the .to_list() above

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'll do do that in a separate PR.


def java_files_with_data(f):
files = java_files(f)
if hasattr(f, "data_runfiles"):
files += list(f.data_runfiles.files)
files += f.data_runfiles.files.to_list()
return files

def _jar_dep_layer_impl(ctx):
Expand Down Expand Up @@ -150,28 +154,24 @@ jar_dep_layer = rule(
def _jar_app_layer_impl(ctx):
"""Appends the app layer with all remaining runfiles."""

available = depset()
for jar in ctx.attr.jar_layers:
available += java_files(jar) # layers don't include runfiles
# layers don't include runfiles
available = depset(transitive = [depset(java_files(jar)) for jar in ctx.attr.jar_layers])

# We compute the set of unavailable stuff by walking deps
# in the same way, adding in our binary and then subtracting
# out what it available.
unavailable = depset()
for jar in ctx.attr.deps + ctx.attr.runtime_deps:
unavailable += java_files_with_data(jar)

unavailable += java_files_with_data(ctx.attr.binary)
unavailable = [x for x in unavailable if x not in available]
unavailable = depset(transitive = [depset(java_files_with_data(jar)) for jar in ctx.attr.deps + ctx.attr.runtime_deps])
unavailable = depset(transitive = [unavailable, depset(java_files_with_data(ctx.attr.binary))])
unavailable = [x for x in unavailable.to_list() if x not in available.to_list()]

# Remove files that are provided by the JDK from the unavailable set,
# as these will be provided by the Java image.
jdk_files = depset(list(ctx.files._jdk))
jdk_files = depset(ctx.files._jdk)
unavailable = [x for x in unavailable if x not in ctx.files._jdk]

classpath = ":".join([
layer_file_path(ctx, x)
for x in available + unavailable
for x in depset(transitive = [available, depset(unavailable)]).to_list()
])

# Classpaths can grow long and there is a limit on the length of a
Expand Down Expand Up @@ -349,19 +349,15 @@ _war_dep_layer = rule(
def _war_app_layer_impl(ctx):
"""Appends the app layer with all remaining runfiles."""

available = depset()
for jar in ctx.attr.jar_layers:
available += java_files(jar)
available = depset(transitive = [depset(java_files(jar)) for jar in ctx.attr.jar_layers])

# This is based on rules_appengine's WAR rules.
transitive_deps = depset()
transitive_deps += java_files(ctx.attr.library)

transitive_deps = depset(java_files(ctx.attr.library))
# TODO(mattmoor): Handle data files.

# If we start putting libs in servlet-agnostic paths,
# then consider adding symlinks here.
files = [d for d in transitive_deps if d not in available]
files = [d for d in transitive_deps.to_list() if d not in available.to_list()]

return _container.image.implementation(ctx, files = files)

Expand Down
Loading