Skip to content

Conversation

svasista-ms
Copy link
Contributor

@svasista-ms svasista-ms commented Aug 14, 2025

Closes #367, closes #479

We previously combined std::fs::canonicalize with a custom strip_extended_path_prefix helper to normalize Windows paths and remove the extended (verbatim) prefix (?). Issue #367 tracked replacing this logic with a standard library alternative, std::path::absolute.

Motivation

  1. Avoid unnecessary filesystem resolution (canonicalize) that can fail or touch the disk when simple normalization is enough.
  2. Rely on stable (std::path::absolute) for syntactic absolutization
  3. Reduce maintenance of custom path utilities.

User Impact:
Slight behavior change in cargo wdk new: verbatim (extended) paths now return a clear error instead of being silently normalized. (Issue #481 was created to add support for verbatim, extended paths)

make `utils` module private and re-export only the required `pub` functions
refactor cargo-wdk and add tests accordingly
@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 05:29
Copilot

This comment was marked as outdated.

@svasista-ms svasista-ms requested a review from a team August 14, 2025 05:31
gurry
gurry previously approved these changes Aug 14, 2025
Copy link
Contributor

@gurry gurry left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @svasista-ms.

I want to suggest that it's better to avoid unrelated changes or those not directly related to the PR.

Copilot

This comment was marked as outdated.

@svasista-ms svasista-ms requested a review from Copilot August 16, 2025 11:36
Copilot

This comment was marked as outdated.

@svasista-ms svasista-ms requested a review from Copilot August 18, 2025 04:16
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

Copy link
Contributor

@gurry gurry left a comment

Choose a reason for hiding this comment

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

Just one more comment. After that I'll approve

@svasista-ms svasista-ms changed the title replace the use of canonicalize + strip_extended_path_prefix with path::absolute refactor: use std::path::absolute instead of canonicalize + strip_extended_path_prefix Aug 26, 2025
fix comment in cli.rs
@svasista-ms svasista-ms dismissed stale reviews from wmmc88, krishnakumar4a4, and gurry via 3f3ed5a August 28, 2025 03:37
Copy link
Contributor

@Copilot 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 refactors the codebase to use the standard library's std::path::absolute function instead of the previous combination of std::fs::canonicalize and a custom strip_extended_path_prefix helper function for path normalization on Windows.

Key changes include:

  • Replace canonicalize + strip_extended_path_prefix with std::path::absolute for syntactic path absolutization
  • Remove custom PathExt trait and related path utility code
  • Add explicit error handling for extended/verbatim paths in cargo wdk new command
  • Move TwoPartVersion type and related utilities from utils module to lib module

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/wdk-build/src/utils.rs Removes PathExt trait, strip_extended_path_prefix utilities, and TwoPartVersion code; changes find_max_version_in_directory visibility
crates/wdk-build/src/lib.rs Adds TwoPartVersion type and utilities; replaces canonicalize+strip calls with std::path::absolute
crates/wdk-build/src/cargo_make.rs Updates path handling to use std::path::absolute instead of canonicalize+strip
crates/cargo-wdk/src/providers/mod.rs Removes PathCanonicalizationError variant
crates/cargo-wdk/src/providers/fs.rs Removes canonicalize_path method from Fs provider
crates/cargo-wdk/src/cli.rs Adds explicit check and error for extended/verbatim paths in new command
crates/cargo-wdk/src/actions/mod.rs Adds PartialEq + Eq derives to Profile and TargetArch enums
crates/cargo-wdk/src/actions/build/tests.rs Removes path canonicalization expectations from test mocks
crates/cargo-wdk/src/actions/build/package_task.rs Adds validation for absolute paths and comprehensive test coverage
crates/cargo-wdk/src/actions/build/mod.rs Updates to use std::path::absolute and removes fs canonicalization calls
crates/cargo-wdk/src/actions/build/error.rs Adds io::Error variant to BuildActionError
crates/cargo-wdk/src/actions/build/build_task.rs Simplifies constructor to use absolute path assertion instead of canonicalization

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

gurry
gurry previously approved these changes Sep 2, 2025
@gurry gurry enabled auto-merge (squash) September 11, 2025 04:05
@gurry gurry merged commit b70ccc0 into microsoft:main Sep 11, 2025
229 checks passed
@gurry gurry deleted the use-path-absolute branch September 11, 2025 04:11
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.

Ensure utility functions from wdk-build are not publicly exported Replace uses of strip_extended_path_prefix with path::absolute
4 participants