-
Notifications
You must be signed in to change notification settings - Fork 694
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
Make transitions optional #2068
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
schemaVersion: 2.0.0 | ||
|
||
fileContentTests: | ||
- name: "validate architecture" | ||
path: "/Files/got_arch.txt" | ||
expectedContents: ["%WANT_ARCH%"] | ||
- name: "validate os" | ||
path: "/Files/got_os.txt" | ||
expectedContents: ["%WANT_OS%"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
schemaVersion: 2.0.0 | ||
|
||
fileContentTests: | ||
- name: "validate architecture" | ||
path: "/Files/got_arch.txt" | ||
expectedContents: ["arm64"] | ||
- name: "validate os" | ||
path: "/Files/got_os.txt" | ||
expectedContents: ["windows"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
""" | ||
Rules for running tests with a specific value of the //transitions:enabled flag | ||
""" | ||
|
||
def _templated_file_impl(ctx): | ||
out = ctx.outputs.out | ||
ctx.actions.expand_template( | ||
template = ctx.file.template, | ||
output = out, | ||
substitutions = dict([s.split("=", 1) for s in ctx.attr.substitutions]), | ||
) | ||
return DefaultInfo( | ||
files = depset([out]), | ||
) | ||
|
||
# Replaces substitutions split on `=` in the template. Substitution is a list | ||
# so that is usable with multiple selects, | ||
templated_file = rule( | ||
attrs = { | ||
"template": attr.label( | ||
mandatory = True, | ||
allow_single_file = True, | ||
), | ||
"substitutions": attr.string_list(), | ||
"out": attr.output(), | ||
}, | ||
implementation = _templated_file_impl, | ||
) | ||
|
||
def _enable_transition_impl(settings, attr): | ||
_ = settings | ||
return {"@io_bazel_rules_docker//transitions:enable": attr.transitions_enabled} | ||
|
||
_enable_transition = transition( | ||
implementation = _enable_transition_impl, | ||
inputs = [], | ||
outputs = [ | ||
"@io_bazel_rules_docker//transitions:enable", | ||
], | ||
) | ||
|
||
def _transitioned_test(ctx): | ||
source_info = ctx.attr.actual[DefaultInfo] | ||
|
||
# Bazel wants the executable to be generated by this rule, let's oblige by | ||
# just copying the actual runner. | ||
executable = None | ||
if source_info.files_to_run and source_info.files_to_run.executable: | ||
executable = ctx.actions.declare_file("{}_{}".format(ctx.file.actual.basename, "on" if ctx.attr.transitions_enabled else "off")) | ||
ctx.actions.run_shell( | ||
command = "cp {} {}".format(source_info.files_to_run.executable.path, executable.path), | ||
inputs = [source_info.files_to_run.executable], | ||
outputs = [executable], | ||
) | ||
return [DefaultInfo( | ||
files = depset(ctx.files.actual), | ||
runfiles = source_info.default_runfiles.merge(source_info.data_runfiles), | ||
executable = executable, | ||
)] | ||
|
||
# Defines a test that runs with a specific value of the | ||
# @io_bazel_rules_docker//transitions:enable flag. | ||
transition_test = rule( | ||
attrs = { | ||
"actual": attr.label( | ||
mandatory = True, | ||
allow_single_file = True, | ||
), | ||
"transitions_enabled": attr.bool(), | ||
"_allowlist_function_transition": attr.label( | ||
default = "@bazel_tools//tools/allowlists/function_transition_allowlist", | ||
), | ||
}, | ||
cfg = _enable_transition, | ||
test = True, | ||
implementation = _transitioned_test, | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# Copyright 2022 The Bazel Authors. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") | ||
|
||
package( | ||
default_visibility = ["//visibility:public"], | ||
) | ||
|
||
bool_flag( | ||
name = "enable", | ||
build_setting_default = True, | ||
) | ||
|
||
config_setting( | ||
name = "enabled", | ||
flag_values = {"@io_bazel_rules_docker//transitions:enable": "true"}, | ||
) | ||
|
||
config_setting( | ||
name = "disabled", | ||
flag_values = {"@io_bazel_rules_docker//transitions:enable": "false"}, | ||
) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hey there @laurynaslubys, First of all thanks a bunch for making these changes and my team has been really interested in something to address some of the overhead issues created by transitions which we think is a nice feature, but mostly not relevant to us because are target and host are always the same . I wanted to mention 2 things:
plaftorm
instead ofplatform
.k8-fastbuild
becomesk8-fastbuild-ST-<someHashHere.
). I'm happy to open an issue, but I would really like if things could behave as before in rules_docker 0.22 where if you didn't explicitly specify something, it didn't create a new output directory for docker images.I noticed that having the
transitions:enable
flag be false causing this function to return an empty JSON object works. But it sounds like your comment says maybe that's an issue for people below Bazel 5.0.This is still a great change and my no means want to downplay it. But at least wanted to share these thoughts.
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.
@cfife-btig Thank you for pointing this out. I spent several hours tracking down the exact same cache miss issue. Patching this to return an empty dict on 0.25.0 and turning transitions off has resolved my issue for now.
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.
Hi @scasagrande ! I just tracked it down today, only our version of Bazel doesn't seem to be compatible with the empty dict solution, so I'm looking at 0.22.
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.
Just a dumb sanity check @abentley-ssimwave but are you setting
io_bazel_rules_docker/transitions:enable=false
on the command-line?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.
@abentley-ssimwave well hello!! Yes I should have included my bazel version, we were on 5.2 at time of my prior comment and we're now up to 5.3.1
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.
That sounds like what should happen. You need to both use the patch and set the flag transitions:enable=false. Sorry if this is redundant, but if you're not doing both of those things together then I don't think it will disable transitions.
If you only set the flag, it just sets the directory
<myoutdir>-hash
to<myoutdir>-somenewhash
because removing those build settings are just generating a new transition hash (when it should really remove the transition has altogether), whereas in 0.22, I believe there were no transitions to begin with.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.
@cfife-btig Yes, I set
--@io_bazel_rules_docker//transitions:enable=false
you can see it in the output I posted.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.
@cfife-btig The result of using the flag with your patch on 3.4.1 is a whole pile of errors, and I do not agree that that "should" happen, although I can accept that there may not be a better solution for 0.25
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.
Hmmm. I'm not sure then. My guess would be your version of Bazel is the issue. I think I'm on 5.2
Basically the patch file returns an empty dict instead of
unset
, and i didn't write it but there's a comment implying it might not be supported in < 5.0 Bazel.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.
@cfife-btig Yes, I agree.