rustbuild: canonicalize prefix install_sh
#49778
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
src/bootstrap/install.rs
Outdated
| fs::canonicalize(p).expect(&format!("could not canonicalize {}", p.display())) | ||
| }; | ||
| let prefix = build.config.prefix.as_ref().map_or(prefix_default, canonicalize); | ||
| let sysconfdir = build.config.sysconfdir.as_ref().map_or(sysconfdir_default, canonicalize); |
There was a problem hiding this comment.
I think we might have to be more careful about sysconfdir here. If it's an absolute path, then canonicalizing it is fine, but if it's relative, it'll be relative to prefix per the documentation, which makes this code (I believe) do the wrong thing.
I think I'd probably branch on is_absolute/is_relative on the path after mapping it to the default to determine if we need to canonicalize it (though it might be true that like all of the other paths, there's never a need to do so?).
There was a problem hiding this comment.
Oh, you're absolutely right. I was confused by the default value being "/etc" rather than "etc". Fixed.
tamird
force-pushed
the
install-relative-prefix
branch
from
1f1a16e
to
cb9d219
Compare
tamird
changed the title
install_shinstall_sh
src/bootstrap/install.rs
Outdated
| @@ -66,13 +66,15 @@ fn install_sh( | |||
| build.info(&format!("Install {} stage{} ({:?})", package, stage, host)); | |||
|
|
|||
| let prefix_default = PathBuf::from("/usr/local"); | |||
| let sysconfdir_default = PathBuf::from("/etc"); | |||
| let sysconfdir_default = PathBuf::from("etc"); | |||
There was a problem hiding this comment.
Er, no, the previous default was correct I think? In that normally you want to have the sysconfdir be /etc and the rest of the dirs be relative to /usr/local. The confusing part of this is that Path::new("/usr/local").join("/etc") evaluates to /etc which is somewhat unexpected, though correct.
There was a problem hiding this comment.
Oh, wow - that is very surprising, but of course, you are right. https://play.rust-lang.org/?gist=9fca8ef12a8ac532ceaa12383983c62f&version=stable
Fixed again!
Testing: ``` $ git diff diff --git a/config.toml.example b/config.toml.example index 9dd3002506..b47bc490cd 100644 --- a/config.toml.example +++ b/config.toml.example @@ -196,7 +196,7 @@ [install] # Instead of installing to /usr/local, install to this path instead. -#prefix = "/usr/local" +prefix = "install-prefix" # Where to install system configuration files # If this is a relative path, it will get installed in `prefix` above $ mkdir install-prefix $ ./x.py install -i --stage 0 --config config.toml.example ... $ ls install-prefix/ bin lib share ``` Closes rust-lang#36989.
tamird
force-pushed
the
install-relative-prefix
branch
from
cb9d219
to
9487902
Compare
|
@bors r+ rollup (can't fail because, well, not tested...) |
|
📌 Commit 9487902 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Apr 9, 2018
rustbuild: canonicalize prefix `install_sh` Testing: ``` $ git diff diff --git a/config.toml.example b/config.toml.example index 9dd3002506..b47bc490cd 100644 --- a/config.toml.example +++ b/config.toml.example @@ -196,7 +196,7 @@ [install] # Instead of installing to /usr/local, install to this path instead. -#prefix = "/usr/local" +prefix = "install-prefix" # Where to install system configuration files # If this is a relative path, it will get installed in `prefix` above $ mkdir install-prefix $ ./x.py install -i --stage 0 --config config.toml.example ... $ ls install-prefix/ bin lib share ``` Closes #36989. r? @Mark-Simulacrum
|
☀️ Test successful - status-appveyor, status-travis |
nodakai
added a commit
to nodakai/rust
that referenced
this pull request
Jun 15, 2020
PR rust-lang#49778 introduced fs::canonicalize() which fails for a nonexistent path. This is a surprise for someone used to GNU Autotools' configure which can create any necessary intermediate directories in prefix. This change makes it run fs::create_dir_all() before canonicalize().
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 16, 2020
…ent-prefix, r=Mark-Simulacrum bootstrap/install.rs: support a nonexistent `prefix` in `x.py install` PR rust-lang#49778 introduced fs::canonicalize() which fails for a nonexistent path. This is a surprise for someone used to GNU Autotools' configure which can create any necessary intermediate directories in prefix. This change makes it run fs::create_dir_all() before canonicalize().
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 19, 2020
…ent-prefix, r=Mark-Simulacrum bootstrap/install.rs: support a nonexistent `prefix` in `x.py install` PR rust-lang#49778 introduced fs::canonicalize() which fails for a nonexistent path. This is a surprise for someone used to GNU Autotools' configure which can create any necessary intermediate directories in prefix. This change makes it run fs::create_dir_all() before canonicalize().
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
Jun 19, 2020
…ent-prefix, r=Mark-Simulacrum bootstrap/install.rs: support a nonexistent `prefix` in `x.py install` PR rust-lang#49778 introduced fs::canonicalize() which fails for a nonexistent path. This is a surprise for someone used to GNU Autotools' configure which can create any necessary intermediate directories in prefix. This change makes it run fs::create_dir_all() before canonicalize().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing:
Closes #36989.
r? @Mark-Simulacrum