Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Dont assume docker path in image util #1262

Merged

Conversation

massemanet
Copy link
Contributor

"image_util.sh" does not respect the docker toolchain.
this PR generates "image_util.sh" from a template instead.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@massemanet
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Nov 5, 2019
@smukherj1
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@smukherj1 smukherj1 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@smukherj1 smukherj1 left a comment

Choose a reason for hiding this comment

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

Oops. Didn't notice there are CI errors. Could you please fix the following?


(12:07:37) ERROR: /workdir/docker/util/run.bzl:176:18: name 'image_utils' is not defined
--
  | (12:07:37) DEBUG: /workdir/cc/image.bzl:79:9: cc_image does not benefit from layers=[], got: [":cc_image_library"]
  | (12:07:37) ERROR: error loading package 'tests/docker/util': Extension 'docker/util/run.bzl' has errors
  | (12:07:37) ERROR: error loading package 'tests/contrib': in /workdir/docker/toolchain_container/toolchain_container.bzl: in /workdir/docker/toolchain_container/debian_pkg_tar.bzl: in /workdir/docker/package_managers/apt_key.bzl: Extension 'docker/util/run.bzl' has errors
  | (12:07:37) ERROR: error loading package 'tests/docker/package_managers': in /workdir/docker/package_managers/apt_key.bzl: Extension 'docker/util/run.bzl' has errors
  | (12:07:37) ERROR: error loading package 'tests/docker/toolchain_container': in /workdir/docker/toolchain_container/toolchain_container.bzl: in /workdir/docker/toolchain_container/debian_pkg_tar.bzl: in /workdir/docker/package_managers/apt_key.bzl: Extension 'docker/util/run.bzl' has errors
  | (12:07:37) ERROR: Skipping '//tests/contrib:derivative_with_volume_repro_test': error loading package 'tests/contrib': in /workdir/docker/toolchain_container/toolchain_container.bzl: in /workdir/docker/toolchain_container/debian_pkg_tar.bzl: in /workdir/docker/package_managers/apt_key.bzl: Extension 'docker/util/run.bzl' has errors


@massemanet massemanet force-pushed the dont-assume-docker-PATH-in-image_util branch from d4f3b25 to 425ff84 Compare November 7, 2019 10:15
@massemanet
Copy link
Contributor Author

tests seemingly passes
/assign @smukherj1

@nlopezgi
Copy link
Contributor

nlopezgi commented Nov 7, 2019

buildkite is still failing with error message:

(10:16:56) ERROR: /workdir/tests/docker/util/BUILD:69:1: in container_run_and_commit rule //tests/docker/util:test_container_commit:
Traceback (most recent call last):
	File "/workdir/tests/docker/util/BUILD", line 69
		container_run_and_commit(name = 'test_container_commit')
	File "/workdir/docker/util/run.bzl", line 185, in _commit_impl
		ctx.actions.expand_template(<4 more arguments>)
	File "/workdir/docker/util/run.bzl", line 200, in ctx.actions.expand_template
		ctx.file.image_utils
No attribute 'image_utils' in file. Make sure there is a label type attribute marked as 'allow_single_file' with this name
Available attributes: _image_utils_tpl, _run_tpl, image

line 200 in run.bzl should probably be
"%{util_script}": image_utils.path,
instead of
"%{util_script}": ctx.file.image_utils.path,

This is a syntax error and you should be able to repro these locally and debug them yourself. Please make sure you can build and test this locally (and that the buildkite tests pass) before pinging again (unless you get stuck and need a hand debugging) as otherwise we will be doing too much back and forth.

@nlopezgi
Copy link
Contributor

nlopezgi commented Nov 8, 2019

/gcbrun

@massemanet
Copy link
Contributor Author

tests seemingly passes
/assign @smukherj1

@massemanet
Copy link
Contributor Author

argh. apologies for messing up your workflow here

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: massemanet, nlopezgi, smukherj1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nlopezgi nlopezgi merged commit 174d7c2 into bazelbuild:master Nov 8, 2019
@massemanet massemanet deleted the dont-assume-docker-PATH-in-image_util branch November 8, 2019 17:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants