Skip to content
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

layered: move configuration into library component #781

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

JakeHillion
Copy link
Contributor

Move the LayerConfig and its children from main.rs into lib.rs. This allows
other tooling, such as config managers or test executors, to modify layered
configs programmatically.

The end goal is to move everything in layered except for the argument parsing
into a run_layered function, but I haven't done it in this diff because it's
a larger change. This is a common pattern in Rust projects to do as little as
possible in main.rs for extensibility.

The only change here, other than publicity and where things are located, is the
signature of CpuPool::alloc_cpus. It previously relied on &Layer, and this
changes it to the two elements of Layer it uses. This allows Layer to stay
confined to main.rs (for now) to prevent scope creep in this PR.

This may be inconvenient in the short term for WIPs and anyone doing non-Cargo
builds (cough me), but having things split into more files should make
rebases/merges easier in the long run.

Test plan:

  • cargo build --release
  • CI.

Move the LayerConfig and its children from `main.rs` into `lib.rs`. This allows
other tooling, such as config managers or test executors, to modify layered
configs programmatically.

The end goal is to move everything in `layered` except for the argument parsing
into a `run_layered` function, but I haven't done it in this diff because it's
a larger change. This is a common pattern in Rust projects to do as little as
possible in `main.rs` for extensibility.

The only change here, other than publicity and where things are located, is the
signature of `CpuPool::alloc_cpus`. It previously relied on `&Layer`, and this
changes it to the two elements of `Layer` it uses. This allows `Layer` to stay
confined to `main.rs` (for now) to prevent scope creep in this PR.

This may be inconvenient in the short term for WIPs and anyone doing non-Cargo
builds (cough me), but having things split into more files should make
rebases/merges easier in the long run.

Test plan:
- `cargo build --release`
- CI.
Copy link
Contributor

@hodgesds hodgesds left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@JakeHillion JakeHillion added this pull request to the merge queue Oct 11, 2024
Merged via the queue into sched-ext:main with commit eb59085 Oct 11, 2024
50 checks passed
@JakeHillion JakeHillion deleted the pr781 branch October 11, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants