-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[infra] Improve srcmap support for Go projects (#3355, #2714). #3664
Conversation
Travis tests have failedHey @Dor1s, TravisBuddy Request Identifier: 6e5a6a70-8036-11ea-ac79-d7a1e92f42eb |
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.
Thanks so much!
infra/gcb/build_and_run_coverage.py
Outdated
}, | ||
] | ||
build_steps = build_lib.project_image_steps(name, image, | ||
project_yaml['language']) |
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.
nit: move project_yaml['language'] to local var language like others. same for other use below.
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.
done
] | ||
build_steps = build_lib.project_image_steps(name, image, | ||
project_yaml['language']) | ||
build_steps.append({ |
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.
Can you add a comment on why msan-builder is only used here ?
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.
done, it's not used in the coverage job because we don't use MSan there
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.
LGTM with a question
PATHS_TO_SCAN="$SRC" | ||
|
||
if [[ $FUZZING_LANGUAGE == "go" ]]; then | ||
PATHS_TO_SCAN="$PATHS_TO_SCAN $GOPATH" |
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.
What's in $GOPATH if this isn't a Go project? Can we use that to determining whether or not to scan it, rather than introducing another env var?
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.
It's always set to /root/go
in the base-builder and always has a few packages we install, so scanning unconditionally will result in irrelevant Go packages revisions in srcmap of any project we build
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.
Thanks for the reviews! I'll merge after testing.
infra/gcb/build_and_run_coverage.py
Outdated
}, | ||
] | ||
build_steps = build_lib.project_image_steps(name, image, | ||
project_yaml['language']) |
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.
done
] | ||
build_steps = build_lib.project_image_steps(name, image, | ||
project_yaml['language']) | ||
build_steps.append({ |
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.
done, it's not used in the coverage job because we don't use MSan there
Travis tests have failedHey @Dor1s, TravisBuddy Request Identifier: e53a8fd0-8054-11ea-a03b-f348d6d5339b |
Tested on GCB (without updated |
Haven't properly tested this yet, please consider it as a draft, but should be fine to review.
I also wanted to pass
FUZZING_LANGUAGE
to thehelper.py shell
, but we can't parse yaml without installingpyyaml
module, so not sure about that, probably not necessary, as users don't typically runsrcmap
command.