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

Fixed Bugs When Not Using Default Target Directory #7266

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

zcg00
Copy link
Contributor

@zcg00 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

@zcg00 zcg00 marked this pull request as draft February 4, 2025 14:26
@zcg00 zcg00 marked this pull request as ready for review February 4, 2025 14:27
Copy link

github-actions bot commented Feb 4, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/usage_vs_getopt (passes in this run but fails in the 'main' branch)

@sylvestre
Copy link
Contributor

some jobs are failing, could you please have a look?

Copy link

github-actions bot commented Feb 4, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@RenjiSann
Copy link
Collaborator

Other than my remark on the fixup commit, it looks OK to me 👍

@zcg00 zcg00 force-pushed the cargo_target_dir branch 2 times, most recently from 62d1d00 to 908d0f9 Compare February 4, 2025 23:44
@RenjiSann
Copy link
Collaborator

There are fails in the CI, I will try to look at them tomorrow if no one does before me.

Copy link

github-actions bot commented Feb 5, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@RenjiSann
Copy link
Collaborator

It looks like the install method using cross does not work. I'm not familiar with this, so I might take some time before I understand what the problem is.

As a reminder, there was actually a warning in build.rs that said it should be tested with cross.

@RenjiSann
Copy link
Collaborator

It looks like the install method using cross does not work. I'm not familiar with this, so I might take some time before I understand what the problem is.

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 cross or not, and to what extent this is tight to my dev setup, but when I run this:

cross +1.79.0 build --release --target=x86_64-unknown-linux-gnu

The OUT_DIR given to the build.rs script is target/x86_64-unknown-linux-gnu/release/deps/.

For some reason, liblibstdbuf.so still gets created in target/release/deps, so that's why the .find() call fails.

@zcg00 zcg00 force-pushed the cargo_target_dir branch 2 times, most recently from e912de6 to 52b8d39 Compare February 5, 2025 23:35
Copy link

github-actions bot commented Feb 6, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

@RenjiSann
Copy link
Collaborator

LGTM 👍 but as I'm not an expert in this domain, I'll let someone cross check before merging.
Maybe @sylvestre ?

Copy link

GNU testsuite comparison:

GNU test failed: tests/test/test. tests/test/test is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre requested a review from Copilot March 2, 2025 08:28
Copy link

@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.

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");

@RenjiSann RenjiSann merged commit 38bbfce into uutils:main Mar 5, 2025
67 of 68 checks passed
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.

3 participants