[build] E: Benchmark specific environment variables #21
Conversation
| @@ -11,6 +11,7 @@ | |||
| // Imports | |||
| //================================================================================================== | |||
|
|
|||
| use std::{fs, io::Read}; | |||
There was a problem hiding this comment.
Could you add a :: Before this? Note that this is the standard used in the project.
| // Get CARGO_MANIFEST_DIR Environment Variable. | ||
| let manifest_dir: String = match env::var("CARGO_MANIFEST_DIR") { | ||
| Ok(mdir) => mdir, | ||
| Err(_) => panic!("failed to get CARGO_MANIFEST_DIR envi"), |
There was a problem hiding this comment.
There is a typo here: "environment".
| if manifest_dir.contains("benchmarks") { | ||
| // Get "benchmarks" directory path. | ||
| let benchmarks_dir = Path::new(&manifest_dir).ancestors() | ||
| .find(|&p| p.ends_with("benchmarks")); | ||
|
|
||
| if let Some(benchmarks_dir) = benchmarks_dir { | ||
| let params_path = benchmarks_dir.join("parameters.json"); | ||
|
|
||
| if params_path.exists() { | ||
| let content = fs::read_to_string(¶ms_path) | ||
| .expect("Failed to read JSON file"); | ||
| // Debug content. | ||
| println("cargo:warning=Read values: {:?}", content); | ||
| // Pass `content` as an environment variable. | ||
| println("cargo:rustc-env=PARAMETERS={}", content); | ||
| } | ||
| } |
There was a problem hiding this comment.
Hard coding logic is always bad. Let's remove the constraint of finding a a file name parameters.json on a benchmarks directory. Instead look for a file in the root directory only. Also, could you rename parameters.json to config.json?
There was a problem hiding this comment.
Also, please add type annotations to all declarations.
There was a problem hiding this comment.
So Inside the current directory, I should look for a file named config.json? So now this config will remain inside a specific benchmark case, right?
There was a problem hiding this comment.
Change it to look for a file named config.json in the root directory, this is generic enough to cover your use case for benchmarks, and we can further leverage that for configuring constants in the project.
There was a problem hiding this comment.
@lemosep you may also want to consider placing the search and replace logic of values in the config.json here in build.rs.
So, if you have a file config.json in the root directory of the project with the following contents:
{
'MATRIX_SIZE': 128
}
You should search and replace all constants in the code base that are named MATRIX_SIZE.
Have you thought doing it differently? If so, how?
There was a problem hiding this comment.
What I've thought first was giving build.rs the JSON parsing logic, but it would require that all other programs contain optional values for serde-json, impractical I might say. So what I've though was to send the JSON file read via std::fs to the application, then, parsed by serde_json crate.
This pull request aims to enhance the build.rs script, so it can pass the required environment variables for benchmark-purpose scenarios. Since all benchmark experiments run in a
no_stdenvironment, there is no OS runtime features asstd::env::var().