Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
|
@dcleoliu , thank you for the contribution. Please sign the eca first, before your contribution can be accepted |
12d41af to
0b96ebf
Compare
d18aa33 to
f07f76a
Compare
src/feo/src/scheduler.rs
Outdated
| impl<const NUM_STEPS: usize> LoopDurationMeter<NUM_STEPS> { | ||
| pub fn track(&mut self, duration: &feo_time::Duration) { | ||
| self.duration_micros += duration.subsec_micros() as usize + 1000000 * duration.as_secs() as usize; | ||
| self.duration_micros += duration.0.subsec_micros() as usize + 1000000 * duration.as_secs() as usize; |
There was a problem hiding this comment.
Duration should provide the subsec_micros method instead
There was a problem hiding this comment.
Good point. I have added a subsec_micros() method to feo_time::Duration and updated the call sites to use it.
src/feo/src/timestamp.rs
Outdated
|
|
||
| // Calculate the startup time of the primary agent | ||
| let startup_time = std::time::SystemTime::UNIX_EPOCH + sync_info.since_epoch.into(); | ||
| let startup_time = std::time::SystemTime::UNIX_EPOCH + sync_info.since_epoch.0; |
There was a problem hiding this comment.
What is wrong with into here? I think we should avoid using direct field access outside of feo_time crate
There was a problem hiding this comment.
I agree that direct field access should be avoided.
I previously used .0 because using .into() here triggered a type inference ambiguity error. Since SystemTime implements Add for multiple duration types(e.g., from std and the time crate), the compiler couldn't resolve the target type for the generic Into<T>.
I've now refactored this to use an explicit core::time::Duration::from(...) conversion.
| data = ["//tests/rust/feo_tests/test_agent"], | ||
| edition = "2024", | ||
| deps = [ | ||
| "@feo_crates//:nix", |
There was a problem hiding this comment.
All the dependencies must be from score_crates
There was a problem hiding this comment.
I've tried switching back to @score_crates//:nix as requested, but the Bazel build for feo_tests failed with unresolved imports for nix::sys::signal::Signal and nix::unistd::Pid. It seems score_crates doesn't currently enable the signal and process features for nix.
Could you share any advice on how I should handle this according to project standards?
There was a problem hiding this comment.
Yes, we need to add missing features directly into score_crates and update the dependency in MODULE.bazel when it's merged and the features are available.
Cargo.toml
Outdated
| tracing = { version = "0.1.41", features = [ | ||
| "attributes", | ||
| ], default-features = false } | ||
| rand = "0.8.5" |
There was a problem hiding this comment.
Some of the crates are downgraded, why is it necessary?
There was a problem hiding this comment.
The downgrades in the previous commit were accidental, likely introduced while I was troubleshooting environment-specific build issues. I have now restored all dependency versions to match main
The only intentional changes remaining are:
- Updating tokio to 1.47.1 (aligned with main) and explicitly enabling the
rt-multi-threadfeature to fix a cargo compilation error in feo-tracer. - Enabling the process feature for nix to support feo_tests.
| bazel_dep(name = "rules_rust_prost", version = "0.67.0") | ||
|
|
||
| # score_crates 0.0.6 only enables ["fs", "mman"] for nix, but feo_tests also needs | ||
| # ["signal", "process"]. Define a local crate repository with the full feature set. |
There was a problem hiding this comment.
This should be handled in score_crates, not in feo
| use alloc::borrow::ToOwned as _; | ||
| use alloc::boxed::Box; | ||
| use feo_com::interface::ActivityInput; | ||
| use score_log::fmt::ScoreDebug; |
There was a problem hiding this comment.
Is it because of editor extension limitations?
There was a problem hiding this comment.
I removed the import because Clippy flagged it as an unused_import, causing the CI build to fail (this build). The ScoreDebug trait is referenced via score_log::fmt::ScoreDebug on line 55, making the top-level import redundant. I think this fits the style better here.
|
@dcleoliu, right now Cargo is not officially supported, because in S-CORE the goal was to use bazel. Maybe you can also split up the PR and raise PRs for the other functional changes/refactorings? |
|
@johannes-esr, thanks for your advice. Yes, I agree that mixing Cargo support might introduce extra complexity and it's better to keep the focus on Bazel. |
Fix cargo build failure. Verified with both cargo and bazel building.