-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 Bash trap framework basics, refactored stack trace #8527
add Bash trap framework basics, refactored stack trace #8527
Conversation
@@ -29,7 +29,8 @@ fi | |||
|
|||
# Build the primary client/server for all platforms | |||
OS_BUILD_PLATFORMS=("${platforms[@]}") | |||
os::build::build_binaries "${OS_CROSS_COMPILE_TARGETS[@]}" | |||
# Create a sub-shell so that we don't pollute the outer environment | |||
( os::build::build_binaries "${OS_CROSS_COMPILE_TARGETS[@]}" ) |
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.
Why not make build_binaries itself run its work in a subshell, rather than propagating up and out?
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.
My question was, also, if we have a work-around for the $GOBIN
thing for cross-compiles, why not use that work-around for everything and never leak a changed $GOBIN
, thereby not needing the subshell at all?
8407af7
to
71b84d1
Compare
@smarterclayton addressed comments. New stack trace:
Old stack trace:
|
[test]me |
Flake on #7429 re[test] |
Conformance flake is #8491 On Mon, Apr 18, 2016 at 2:25 PM, OpenShift Bot [email protected]
|
71b84d1
to
9af7f9f
Compare
local bash_source="$( os::util::repository_relative_path "${BASH_SOURCE[$i]}" )" | ||
if [[ -z "${preamble_finished-}" ]]; then | ||
preamble_finished=true | ||
os::log::error "PID ${BASHPID:-$$}: ${bash_source}:${BASH_LINENO[$i-1]}: \`${last_command}\` exited with status ${return_code}." >&2 |
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.
@Miciah I understand ${BASHPID:-$$}
isn't technically a loss-less default, but I'm not particularly interested in making this nice for people using 12-year-old-Bash, so instead of a clear indication that each stacktrace is for a failed process, they'll just always see the parent PID.
integration flake: #8573 re[test] |
local i | ||
for (( i = stack_begin_index; i < ${#BASH_SOURCE[@]}; i++ )); do | ||
local bash_source="$( os::util::repository_relative_path "${BASH_SOURCE[$i]}" )" | ||
if [[ -z "${preamble_finished-}" ]]; then |
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.
missing :
? (it doesn't actually matter I guess, but it is more consistent)
9af7f9f
to
c96500e
Compare
( os::build::internal::build_binaries "$@" ) | ||
} | ||
|
||
# Build binaries targets specified. Should always be run in a sub-shell so we don't leak GOBIN |
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.
Seems like "binaries targets" here and on line 261 should be "binary targets" instead.
Nothing uses |
source "${OS_ROOT}/hack/lib/util/trap.sh" | ||
source "${OS_ROOT}/hack/lib/log/stacktrace.sh" | ||
|
||
trap os::test::junit::reconcile_output EXIT |
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 looks like a fix that is not directly related to the other changes, or am I mistaken?
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.
Yes, good eyes - I just caught this while I was fixing preambles.
Reviewed, commented on some minor things. Sorry for the delay! |
997c1e2
to
835a39f
Compare
AWS Infra flake:
re[test] |
835a39f
to
830a7f7
Compare
re[test] |
This changes the prior implementation of the stacktrace to use the new stacktrace implementation as well as the newly-implemented Bash trap framework for the ERR signal. In order to make the stacktrace output more useful, especially for someone attempting to re-run the commands used, we evaluate all of the variables in the $BASH_COMMAND as best we can. We will not be able to ever correctly evaluate positional variables ($1, $2, $@) or variables created in the local scopes of the trap handler or in the stacktrace function such as $return_code, $last_command, or $errexit_set. For this reason, we print both $BASH_COMMAND and our attempt to substitute variables. Signed-off-by: Steve Kuznetsov <[email protected]>
By changing the amount of code that is literally contained in the sub-shell `()` braces, we allow the stacktrace that is generated to contain much less text on any one line, so that it is read-able and useful. Furthermore, by naming the positional variables we pass around we allow the new stack trace code to correctly evaluate them and show us what went wrong, exactly. Signed-off-by: Steve Kuznetsov <[email protected]>
830a7f7
to
62e2e0a
Compare
@smarterclayton this is ready
|
Well, there was a green run before. Not sure what kicked this one. e2e flake:
test-cmd flake:
re[test] |
Looks like a new flake in conformance and integration:
re[test] |
Extended failures:
@smarterclayton you seen this before? ^^ re[test] |
@mfojtik @smarterclayton this looks like a rebase issue?
|
it seems so, label it as post rebase, I will have a look tomorrow. Sent from my phone On 16 Jun 2016, at 23:42, Steve Kuznetsov [email protected] wrote: @mfojtik https://github.com/mfojtik @smarterclayton I0616 16:05:52.554723 19712 meta.go:46] Calling Accessor on goroutine 5893 [running]: — |
re[test] |
flake 2x #8571 re[test] |
Hit #8571 again re[test] |
hit #9457 |
hit #5448 re[test] |
Evaluated for origin test up to 62e2e0a |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5194/) |
LGTM [merge] |
Evaluated for origin merge up to 62e2e0a |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5194/) (Image: devenv-rhel7_4437) |
hit #5448 |
I never thought the day would come. |
@stevekuznetsov I know that feeling, congrats! |
Who knows, maybe one day it will become https://github.com/niieani/bash-oo-framework#error-handling-with-exceptions-and-throw... |
@rhcarvalho good lord ... |
This will begin the series of pulls that hopefully will bring in the trap framework.
@Miciah please (re-)review the Bash bits
@smarterclayton please review the last commit