-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixed Bugs When Not Using Default Target Directory #7266
Conversation
zcg00
commented
Feb 4, 2025
- Changes stdbuf's build script to allow target directory to not require 'target' or 'cargo-install' in name
- Updates scripts to use "CARGO_TARGET_DIR" when supplied
GNU testsuite comparison:
|
some jobs are failing, could you please have a look? |
GNU testsuite comparison:
|
Other than my remark on the fixup commit, it looks OK to me 👍 |
62d1d00
to
908d0f9
Compare
There are fails in the CI, I will try to look at them tomorrow if no one does before me. |
GNU testsuite comparison:
|
It looks like the install method using As a reminder, there was actually a warning in build.rs that said it should be tested with cross. |
OK so I don't know if this is expected behavior from
The For some reason, |
e912de6
to
52b8d39
Compare
GNU testsuite comparison:
|
LGTM 👍 but as I'm not an expert in this domain, I'll let someone cross check before merging. |
GNU testsuite comparison:
|
067c8bb
to
07bed67
Compare
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR fixes the process of locating the dependencies directory during the build, allowing the target directory to have a flexible name (i.e. not requiring "target" or "cargo-install").
- Replaces the iterative parent lookup from OUT_DIR with a lookup based on the executable's ancestry.
- Updates the file copy logic to reflect the new directory resolution.
Reviewed Changes
File | Description |
---|---|
src/uu/stdbuf/build.rs | Revised logic for determining the deps directory using current_exe and ancestors |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/uu/stdbuf/build.rs:33
- Using nth(3) from current_exe ancestors may be brittle if the executable path structure changes; consider adding error handling or a more robust method to determine the dependencies directory.
let deps_dir = current_exe.ancestors().nth(3).unwrap().join("deps");
2433e4a
to
43036e2
Compare