Skip to content

xsync: decouple from oss workspace#3209

Draft
benhillis wants to merge 1 commit intomicrosoft:mainfrom
benhillis:user/benhill/xsync-decouple
Draft

xsync: decouple from oss workspace#3209
benhillis wants to merge 1 commit intomicrosoft:mainfrom
benhillis:user/benhill/xsync-decouple

Conversation

@benhillis
Copy link
Copy Markdown
Member

@benhillis benhillis commented Apr 6, 2026

Remove the cross-workspace path dependency on ci_logger (which resolved its workspace = true deps against the oss workspace), making xsync fully self-contained. This ensures xsync can build and run even when the parent workspace is broken — eliminating the chicken-and-egg problem where xsync is needed to regenerate Cargo.toml but can't build because the workspace is invalid.

The ADO-aware logging is reimplemented in a small log_init module (~60 lines) with no external dependencies beyond log, replacing both ci_logger and env_logger.

No pipeline changes needed — update-mirror.yaml and hvlite-pr.yaml already invoke xsync directly from oss/xsync/.

Copilot AI review requested due to automatic review settings April 6, 2026 21:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR makes the xsync tool self-contained by removing its cross-workspace path dependency on ci_logger and inlining the small logger implementation directly into the xsync crate, so xsync can still build even if the parent workspace is temporarily broken.

Changes:

  • Added a local ci_logger module in xsync by inlining the logger implementation.
  • Replaced the ci_logger path/workspace dependency with a direct env_logger dependency in the xsync/ nested workspace.
  • Updated Cargo.lock to remove the ci_logger package entry and reflect the new direct dependency.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
xsync/xsync/src/main.rs Registers the new local ci_logger module for initialization at startup.
xsync/xsync/src/ci_logger.rs Inlines the ADO-aware logger implementation previously sourced from support/ci_logger.
xsync/xsync/Cargo.toml Switches dependency from ci_logger to env_logger (workspace-provided) for the inlined module.
xsync/Cargo.toml Removes the cross-workspace path dependency and adds env_logger to the nested workspace dependencies.
xsync/Cargo.lock Removes the ci_logger package and updates xsync’s dependency list accordingly.

@benhillis benhillis force-pushed the user/benhill/xsync-decouple branch from e1c02f3 to cccf610 Compare April 6, 2026 21:57
@benhillis benhillis changed the title xsync: inline ci_logger to decouple from oss workspace xsync: decouple from oss workspace Apr 6, 2026
Remove the cross-workspace path dependency on ci_logger (which resolved
its workspace deps against the oss workspace), making xsync fully
self-contained. This ensures xsync can build and run even when the
parent workspace is broken -- eliminating the chicken-and-egg problem
where xsync is needed to fix the workspace but can't build because the
workspace is invalid.

The ci_logger code (~90 lines) is inlined as a local module since xsync
only uses the init() function.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@benhillis benhillis force-pushed the user/benhill/xsync-decouple branch from cccf610 to 30add36 Compare April 7, 2026 20:54
Copilot AI review requested due to automatic review settings April 7, 2026 20:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +20 to +28
fn new(log_level: Option<&str>) -> AdoLogger {
let max_level = match log_level.map(|s| s.to_lowercase()).as_deref() {
Some("trace") => LevelFilter::Trace,
Some("debug") => LevelFilter::Debug,
Some("info") => LevelFilter::Info,
Some("warn" | "warning") => LevelFilter::Warn,
Some("error") => LevelFilter::Error,
Some("off") => LevelFilter::Off,
_ => LevelFilter::Info,
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

XSYNC_LOG used to accept full env_logger filter syntax via ci_logger (e.g. module=debug,other=warn). This reimplementation only recognizes a few exact strings and silently falls back to Info for anything else, which can make existing configs appear to “stop working”. Consider either (a) supporting a minimal subset of the previous filter syntax (at least target=level / comma-separated), or (b) treating unrecognized values as an error (or at least printing a one-time warning to stderr before installing the logger) so misconfiguration is visible.

Suggested change
fn new(log_level: Option<&str>) -> AdoLogger {
let max_level = match log_level.map(|s| s.to_lowercase()).as_deref() {
Some("trace") => LevelFilter::Trace,
Some("debug") => LevelFilter::Debug,
Some("info") => LevelFilter::Info,
Some("warn" | "warning") => LevelFilter::Warn,
Some("error") => LevelFilter::Error,
Some("off") => LevelFilter::Off,
_ => LevelFilter::Info,
fn parse_level_filter(log_level: &str) -> Option<LevelFilter> {
match log_level.trim().to_ascii_lowercase().as_str() {
"trace" => Some(LevelFilter::Trace),
"debug" => Some(LevelFilter::Debug),
"info" => Some(LevelFilter::Info),
"warn" | "warning" => Some(LevelFilter::Warn),
"error" => Some(LevelFilter::Error),
"off" => Some(LevelFilter::Off),
_ => None,
}
}
fn new(log_level: Option<&str>) -> AdoLogger {
let max_level = match log_level {
Some(value) => match Self::parse_level_filter(value) {
Some(level) => level,
None => {
eprintln!(
"warning: unrecognized log level {:?}; falling back to 'info'. Supported values: trace, debug, info, warn, warning, error, off.",
value
);
LevelFilter::Info
}
},
None => LevelFilter::Info,

Copilot uses AI. Check for mistakes.
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