-
-
Notifications
You must be signed in to change notification settings - Fork 12.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
rustup-init 1.3.0 (new formula) #14236
Conversation
Formula/rustup.rb
Outdated
rustup-init | ||
|
||
EOS | ||
end |
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.
These caveats
can all be removed as they are not Homebrew specific, thanks.
Formula/rustup.rb
Outdated
end | ||
|
||
test do | ||
assert_equal "rustup-init 1.3.0 ( )", |
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.
Embed #{version}
so this doesn't need updated for every release. Alternatively, use assert_match
and don't match on the version at all (and remove the .strip
)
Formula/rustup.rb
Outdated
|
||
test do | ||
assert_equal "rustup-init 1.3.0 ( )", | ||
shell_output("#{bin/"rustup-init"} -V").strip |
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.
"#{bin}/rustup-init -V"
I've made the requested changes, but
|
Formula/rustup.rb
Outdated
homepage "https://github.com/rust-lang-nursery/rustup.rs" | ||
|
||
url "https://github.com/rust-lang-nursery/rustup.rs/archive/1.3.0.tar.gz" | ||
version "1.3.0" |
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.
You can remove this line.
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.
Yes, I misread the "#{version}" comment… sorry about that.
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.
Not sure I understand this PR as-is. My understanding is that rustup-init is not itself rustup.
|
It's not, If it is permitted, I can add environment variables to the formula and run |
What exactly does running |
We can manually do so in the formula, but I avoided doing so… because it seemed too invasive for brew to do so in the user's home directory. |
What will |
My $0.02 is that |
What value is being able to run
adding as opposed to running the upstream recommended command
|
From my own perspective, it's far more preferable to install via my existing package manager. Would you rather I change the formula to invoke rustup-init into |
The way I would expect this to work as a Homebrew formula would be:
That's largely how rbenv works, except it's a shell script that's But instead we have to install the shell script So I guess my question is, is it possible to bypass these |
@dunn when you run rustup-init (which is a compiled rust program), it actually copies itself into ~/.cargo/bin with the name rustup, and makes hardlinks of itself with various other names in ~/.cargo/bin as well, and then it downloads a bunch of precompiled stuff that goes in ~/.cargo and ~/.multirust. One problem with installing any of it into It is simple to have things go somewhere other than ~/.cargo by running
where somewhere could be say However that will break because the shell scripts names will again conflict with the main rust formula, and because the binaries in |
@ilovezfs Can the formula spec express a |
Technically it could because |
An alternative would be to use something other than the Homebrew rust formula to build rustup, which could be, for example, the bootstrap binaries we use to build However, we'd still have the problem of the downloaded toolchains being stranded in |
So, a potential path forward would be:
|
That's not necessary, since you can just set CARGO_HOME and RUSTUP_HOME to wherever you want them to end up, rather than first staging them in the buildpath. |
Woops, posted without refreshing the page. |
I'm not clear on why this has to conflict with the class Rustup < Formula
desc "The Rust toolchain installer"
homepage "https://github.com/rust-lang-nursery/rustup.rs"
url "https://github.com/rust-lang-nursery/rustup.rs/archive/1.3.0.tar.gz"
sha256 "c0ca06b70104fed8f1de5a6f5ecfd8478e8bc03f15add8d7896b86b3b15e81e3"
head "https://github.com/rust-lang-nursery/rustup.rs.git"
depends_on "rust" => :build
def install
system "cargo", "build", "--release"
bin.install "target/release/rustup-init" => "rustup"
end
test do
system bin/"rustup", "toolchain", "install", "stable"
end
end Then the user can set the env variables to With the above formula I can
|
If you install it with the name |
Yeah, if you install it with the name |
@dunn regarding conflicts, once
And the
So anything that results in the fully installed This is why multirust had |
This is how I meant we could get around the conflict, but it runs into the headerpad issue: def install
ENV["RUSTUP_HOME"] = pkgshare
ENV["CARGO_HOME"] = pkgshare
system "cargo", "build", "--release"
system "target/release/rustup-init", "--no-modify-path", "-y"
(bin/"rustup").write_env_script(pkgshare/"bin/rustup",
:RUSTUP_HOME => pkgshare,
:CARGO_HOME => pkgshare)
end This is the error we get during install:
Will that matter, if they add |
@dunn now we're on the same page (except I had used libexec instead of pkgshare).
They wouldn't need to because rustup itself handles finding it, but we'd still need to resolve the header padding issue. But even if we resolved that, you still have the problem of the toolchains getting dropped every time the Cellar changes unless everything is put in You also have the problem that it's not possible for the user to set a different CARGO_HOME and RUSTUP_HOME unless they re-run rustup-init in libexec or pkgshare, which we'd also need to install. You wouldn't be able to just set them in your profile since a) they require rustup-init to be run again and b) they'd be overridden by the env scripts. I think given all of the other problems, naming the formula I'm still not convinced that adds any value beyond just having people do |
Oh, I agree. |
Aliases/rustup.rb
Outdated
@@ -0,0 +1 @@ | |||
../Formula/rustup-init.rb |
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.
The alias itself shouldn't have a .rb extension
Formula/rustup-init.rb
Outdated
end | ||
|
||
test do | ||
assert_match "rustup-init #{version} ( )", |
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.
Maybe
test do
system bin/"rustup-init", "-y"
(testpath/"hello.rs").write <<-EOS.undent
fn main() {
println!("Hello World!");
}
EOS
system testpath/".cargo/bin/rustc", "hello.rs"
assert_equal "Hello World!", shell_output("./hello").chomp
end
Great work @ChristopherMacGown! Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock! |
As I'm late in discovering this discussion, I don't know if it's more appropriate to continue the discussion on this PR, or if a new one is called for, but in any case... The current Second, the formula's bottles are trivially relocatable. The Third, I don't think The error @dunn mentioned in his comment about Other Homebrew packages which work similarly include Below is a formula which takes care of these problems (except I'm not 100% sure about the bottle DSL — does the build system take insert the necessary checksums?), and, IMO, installs more cleanly than the current one. It should be just called
|
I don't think a new PR is needed.
That is intentional.
This is not the case as they do not pass the bottle checks for relocatibility.
This is why you should use the standard Homebrew prefix. If you don't, then you're probably best off using upstream's init script if you're not willing to wait for rust to build.
That is how the software works. If you disagree with it, you'd need to take it up with upstream not Homebrew.
This is the default behavior of the software. If you want it somewhere else, you're welcome to set the relevant environment variables in your profile.
I think the existing formula is fine. |
How so? The output for
This is the only binary file installed by the formula. The others only provide shell auto-completion and also make no references to a brew prefix location. |
You can |
Sample output from your suggestion:
All string matches refer to the |
@gcv you're welcome to open a PR to Homebrew/brew to attempt to improve the test for relocatability, as the generation of the bottle blocks is entirely automated, but, if anything, you're making me think we should just make rust a run-time dependency as well. |
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install <formula>
)?rustup is a multiplexing toolchain installer for rust similar to pyenv, rbenv, etc…
I think this PR addresses the concerns raised in #9617.
It pulls source from the stable release tarball, uses brew rust to install from source, and installs the rustup-init executable into the keg.
rustup-init
installs intoRUSTUP_HOME
which defaults to ~/.multirust.