Skip to content

rust: Ensure cargo executable path is fetched for custom targets when bootstrapping rust compiler #122069

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

rust: Ensure cargo executable path is fetched for custom targets when bootstrapping rust compiler #122069

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/bootstrap/bootstrap.py
View file
Original file line number Diff line number Diff line change
@@ -975,11 +975,15 @@ def build_bootstrap_cmd(self, env):
if "RUSTFLAGS_BOOTSTRAP" in env:
env["RUSTFLAGS"] += " " + env["RUSTFLAGS_BOOTSTRAP"]

env["PATH"] = os.path.join(self.bin_root(), "bin") + \
cargo_bin_path = os.path.join(self.bin_root(), "bin") + \
os.pathsep + env["PATH"]
if not os.path.isfile(self.cargo()):
raise Exception("no cargo executable found at `{}`".format(
if cargo_bin_path is None:
raise Exception("no cargo executable found at `{}`".format(
self.cargo()))
else:
cargo_bin_path = os.getenv("RUST_TARGET_PATH") + "rust-snapshot/bin/cargo"
Copy link
Member

@onur-ozkan onur-ozkan Mar 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't include custom paths in the source code directly. Why not adding it to "PATH" in your environment?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for looking into the PR.

I thought of including the path set by RUST_TARGET_PATH + "rust-snapshot/bin/cargo" and adding it as cargo bin path.

Why not adding it to "PATH" in your environment?

While doing rustc build I don't know the path of cargo bin beforehand and the build gives error as mentioned in #122069 (comment)

Should I do any changes in the above patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you know that Cargo will be at $RUST_TARGET_PATH + "rust-snapshot/bin/cargo", can't you just add that to your PATH in your build process? Or set cargo in config.toml to that

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I am running the build process with a wrapper script. $RUST_TARGET_PATH is set after the rust sources are generated and while build is running. If I add it in the PATH before build, it would fail as $RUST_TARGET_PATH is undefined path. Still, I will give it a try and check as you mentioned and get back.

Copy link
Member

Choose a reason for hiding this comment

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

I am quite sure there are plenty of ways to solve this problem without adding hard-coded custom paths in the source code. If not, then your build flow is obviously doing something very wrong. We can't accept this change to upstream.

Copy link
Author

Choose a reason for hiding this comment

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

Does the following changes look good to you?

Reverting the changes on line no 978. Then using that cargo_bin_path value to handle cases where cargo bin is not found(as in my case, since this is been handled at very last else case).

diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py
index 0d604c0d3e5..e7224ab45b8 100644
--- a/src/bootstrap/bootstrap.py
+++ b/src/bootstrap/bootstrap.py
@@ -977,9 +977,15 @@ class RustBuild(object):
 
         env["PATH"] = os.path.join(self.bin_root(), "bin") + \
             os.pathsep + env["PATH"]
+        cargo_bin_path = os.path.join(self.bin_root(), "bin") + \
+            os.pathsep + env["PATH"]
         if not os.path.isfile(self.cargo()):
-            raise Exception("no cargo executable found at `{}`".format(
+            if cargo_bin_path is None:
+                raise Exception("no cargo executable found at `{}`".format(
                 self.cargo()))
+            else:
+                cargo_bin_path = os.getenv("RUST_TARGET_PATH") + "rust-snapshot/bin/cargo"
+                env["PATH"] = os.path.dirname(cargo_bin_path) + os.pathsep + env["PATH"]
         args = [self.cargo(), "build", "--manifest-path",
                 os.path.join(self.rust_root, "src/bootstrap/Cargo.toml")]
         args.extend("--verbose" for _ in range(self.verbose))

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned earlier, we can't accept hard-coded paths for specific projects like this rust-snapshot path. This really sounds like this would be better solved within your build flow

env["PATH"] = os.path.dirname(cargo_bin_path) + os.pathsep + env["PATH"]
args = [self.cargo(), "build", "--manifest-path",
os.path.join(self.rust_root, "src/bootstrap/Cargo.toml")]
args.extend("--verbose" for _ in range(self.verbose))