From 4b14048d6037a2da5601952a2dd36fe67d04218b Mon Sep 17 00:00:00 2001
From: onur-ozkan <work@onurozkan.dev>
Date: Mon, 30 Oct 2023 11:22:32 +0300
Subject: [PATCH] improve and fix x install

Fix: Write access check of `prefix` and `sysconfdir`
when DESTDIR is present.

Improvement: Instead of repeatedly reading `DESTDIR` within
each `fn prepare_dir` usage, read it once and pass it to
the `fn prepare_dir`.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
---
 src/bootstrap/src/core/build_steps/install.rs | 52 +++++++++++--------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/src/bootstrap/src/core/build_steps/install.rs b/src/bootstrap/src/core/build_steps/install.rs
index 391995b7c3b84..ed6b1c5154c8c 100644
--- a/src/bootstrap/src/core/build_steps/install.rs
+++ b/src/bootstrap/src/core/build_steps/install.rs
@@ -71,16 +71,27 @@ fn install_sh(
 
     let prefix = default_path(&builder.config.prefix, "/usr/local");
     let sysconfdir = prefix.join(default_path(&builder.config.sysconfdir, "/etc"));
+    let destdir_env = env::var_os("DESTDIR").map(PathBuf::from);
 
-    // Sanity check for the user write access on prefix and sysconfdir
-    assert!(
-        is_dir_writable_for_user(&prefix),
-        "User doesn't have write access on `install.prefix` path in the `config.toml`.",
-    );
-    assert!(
-        is_dir_writable_for_user(&sysconfdir),
-        "User doesn't have write access on `install.sysconfdir` path in `config.toml`."
-    );
+    // Sanity checks on the write access of user.
+    //
+    // When the `DESTDIR` environment variable is present, there is no point to
+    // check write access for `prefix` and `sysconfdir` individually, as they
+    // are combined with the path from the `DESTDIR` environment variable. In
+    // this case, we only need to check the `DESTDIR` path, disregarding the
+    // `prefix` and `sysconfdir` paths.
+    if let Some(destdir) = &destdir_env {
+        assert!(is_dir_writable_for_user(destdir), "User doesn't have write access on DESTDIR.");
+    } else {
+        assert!(
+            is_dir_writable_for_user(&prefix),
+            "User doesn't have write access on `install.prefix` path in the `config.toml`.",
+        );
+        assert!(
+            is_dir_writable_for_user(&sysconfdir),
+            "User doesn't have write access on `install.sysconfdir` path in `config.toml`."
+        );
+    }
 
     let datadir = prefix.join(default_path(&builder.config.datadir, "share"));
     let docdir = prefix.join(default_path(&builder.config.docdir, "share/doc/rust"));
@@ -94,13 +105,13 @@ fn install_sh(
     let mut cmd = Command::new(SHELL);
     cmd.current_dir(&empty_dir)
         .arg(sanitize_sh(&tarball.decompressed_output().join("install.sh")))
-        .arg(format!("--prefix={}", prepare_dir(prefix)))
-        .arg(format!("--sysconfdir={}", prepare_dir(sysconfdir)))
-        .arg(format!("--datadir={}", prepare_dir(datadir)))
-        .arg(format!("--docdir={}", prepare_dir(docdir)))
-        .arg(format!("--bindir={}", prepare_dir(bindir)))
-        .arg(format!("--libdir={}", prepare_dir(libdir)))
-        .arg(format!("--mandir={}", prepare_dir(mandir)))
+        .arg(format!("--prefix={}", prepare_dir(&destdir_env, prefix)))
+        .arg(format!("--sysconfdir={}", prepare_dir(&destdir_env, sysconfdir)))
+        .arg(format!("--datadir={}", prepare_dir(&destdir_env, datadir)))
+        .arg(format!("--docdir={}", prepare_dir(&destdir_env, docdir)))
+        .arg(format!("--bindir={}", prepare_dir(&destdir_env, bindir)))
+        .arg(format!("--libdir={}", prepare_dir(&destdir_env, libdir)))
+        .arg(format!("--mandir={}", prepare_dir(&destdir_env, mandir)))
         .arg("--disable-ldconfig");
     builder.run(&mut cmd);
     t!(fs::remove_dir_all(&empty_dir));
@@ -110,19 +121,16 @@ fn default_path(config: &Option<PathBuf>, default: &str) -> PathBuf {
     config.as_ref().cloned().unwrap_or_else(|| PathBuf::from(default))
 }
 
-fn prepare_dir(mut path: PathBuf) -> String {
+fn prepare_dir(destdir_env: &Option<PathBuf>, mut path: PathBuf) -> String {
     // The DESTDIR environment variable is a standard way to install software in a subdirectory
     // while keeping the original directory structure, even if the prefix or other directories
     // contain absolute paths.
     //
     // More information on the environment variable is available here:
     // https://www.gnu.org/prep/standards/html_node/DESTDIR.html
-    if let Some(destdir) = env::var_os("DESTDIR").map(PathBuf::from) {
-        // Sanity check for the user write access on DESTDIR
-        assert!(is_dir_writable_for_user(&destdir), "User doesn't have write access on DESTDIR.");
-
+    if let Some(destdir) = destdir_env {
         let without_destdir = path.clone();
-        path = destdir;
+        path = destdir.clone();
         // Custom .join() which ignores disk roots.
         for part in without_destdir.components() {
             if let Component::Normal(s) = part {