Skip to content
This repository has been archived by the owner on Dec 24, 2024. It is now read-only.
/ build Public archive

[build] E: Benchmark specific environment variables #21

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions user/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
// Imports
//==================================================================================================

use std::{fs, io::Read};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a :: Before this? Note that this is the standard used in the project.

use ::std::{
env,
path::Path,
Expand All @@ -35,6 +36,35 @@ fn main() {
Err(_) => panic!("failed to get OUT_DIR environment variable"),
};

//==============================================================================================
//Benchmark-Specific Environment Variables
//==============================================================================================

// 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"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(&params_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);
}
}
Comment on lines +49 to +65
Copy link
Contributor

@ppenna ppenna Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please add type annotations to all declarations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ppenna ppenna Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

//==============================================================================================
// Configure Toolchain
//==============================================================================================
Expand Down