-
Notifications
You must be signed in to change notification settings - Fork 722
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
ci: Criterion benchmark handlers #3223
Conversation
b115b59
to
1f142cd
Compare
let s2nc_env: &str = &env::var("S2NC_ARGS").unwrap(); | ||
let s2nc_args: utils::Arguments = s2nc_env.into(); | ||
dbg!("s2nc harness: {:?}", &s2nc_args); | ||
let output = Command::new("/usr/local/bin/s2nc") |
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 hard coded path means that s2nd/c need to be installed globally. May make this an environment variable?
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.
in my initial attempts at this, PATH was getting modified by tox/pytest, and it was easier to install these, since the default CMAKE_INSTALLPREFIX was left untouched in the path
75bf8cf
to
f294e54
Compare
f294e54
to
668ac8c
Compare
668ac8c
to
89f1d34
Compare
77dcafe
to
a24f115
Compare
Resolved issues:
part of #3130
Description of changes:
Changes to the Criterion benchmarks allow a criterion to call s2nc or s2nd.
part 4 of 5. This only affects integrationv2 tests when run with the
integrationv2crit
flag (once all are merged).Call-outs:
s2nc
ands2nd
must be in the path.Testing:
How is this change tested (unit tests, fuzz tests, etc.)? ci
Is this a refactor change? not exactly
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.