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

Fix: Allow Nextest to Work with Manifests Outside of Workspace Root Path (nextest-rs/nextest issue #1684) v2 #331

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

PegasusPlusUS
Copy link

Resubmitting for previous one polluted by huge background file commits in GitHub codespace.

Fix: Allow Nextest to Work with Manifests Outside of Workspace Root Path
Description
This pull request addresses an issue where Nextest fails to function correctly when the Cargo.toml manifest file is located outside of the workspace root path. The changes ensure that Nextest can handle such scenarios gracefully.

Changes
Updated the guppy crate to use pathdiff for relative path of manifest from workspace root, no matter it is outside of the workspace root.

Modified guppy/src/graph/build.rs fn workspace_path() #402~#408 , add 3 test cases on relative path result for unix and windodws.
Remove debug assert for relative path in fn convert_forward_slashes() #990-#994 so that if manifest path may be on another driver on windows.

Issue
This pull request fixes nextest-rs/nextest issue #1684.

Testing
Verified that Nextest can now correctly locate and use Cargo.toml files outside the workspace root.

Ran existing test suites to ensure no regressions were introduced.

@PegasusPlusUS
Copy link
Author

PegasusPlusUS commented Nov 16, 2024

Before patch, cargo t can work, cargo nextest run can't work:
Screenshot 2024-11-16 134010
Using patched guppy, now cargo nextest run can test normally:
Screenshot 2024-11-16 134151
workspace on C:, manifest on D:
Screenshot 2024-11-16 142345

@PegasusPlusUS
Copy link
Author

Don't know why there still has background commit that contained in PR, which I can't see these files before submitting PR. Hope this time these small amout of files (compared to previously huge amount of files) not affecting review and merge process.

guppy/src/graph/build.rs Outdated Show resolved Hide resolved
guppy/src/graph/build.rs Outdated Show resolved Hide resolved
guppy/src/graph/build.rs Outdated Show resolved Hide resolved
@PegasusPlusUS
Copy link
Author

Updated, test all passed on Ubuntu and Windows, sample workspace on Ubuntu and Windows, and extra workspace that cross drives on Windows all passed.

guppy/src/graph/build.rs Outdated Show resolved Hide resolved
guppy/src/graph/build.rs Outdated Show resolved Hide resolved
guppy/src/graph/build.rs Outdated Show resolved Hide resolved
@@ -548,7 +546,7 @@ impl PackageSourceImpl {
let path_diff = if path_diff.is_absolute() {
path_diff
} else {
convert_forward_slashes(path_diff)
convert_relative_forward_slashes(path_diff)
Copy link
Author

Choose a reason for hiding this comment

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

Since convert_forward_slashes() will convert relative path and clone absolute path,
these 6 lines of code can be done as 1 line:

Self::Path(convert_relative_forward_slashes(path_diff).into_boxed_path())

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