-
Notifications
You must be signed in to change notification settings - Fork 119
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
chore(rust): Install rust via rustup #2796
base: main
Are you sure you want to change the base?
Conversation
7ab9045
to
9a18184
Compare
In general, we prefer to build the binaries rather than vendoring them, unless there are real technical limitations that cannot be overcome. Is it not feasible to populate the sources (and provide a runtime environment variable, and prevent the version annotation) via our build method? I believe, at one point, we did manage to prevent the -dev annotation, but that might have bit rotted. @mxcl any thoughts on this? |
Yeah, I’m aware you prefer to build but 1) I think as the official rust installer this should be trustworthy and 2) the dev annotation is not the only issue. There’s a bit more in the issue I linked, but there are other components that are not getting built and included. While we can likely find way to patch things up, it seemed to me that was no longer as reliable. If you look at the first commit in this PR, you’ll see how I was first trying to get the src component installed. It was very manual and prone to issues if rust were to change their repo structure. This seems more reliable IMO |
Why do package managers typically build from source?
3 is important, which is why we have (so far) (mostly) avoided vendoring and I have generally said “let’s support vendored binaries but I want them to be signed” though so far we haven't enforced that because so few maintainers provide signatures. The rust maintainers want people to use So probably we should use |
yeah we should merge this. HOWEVER, is there some kind of signature, like even a SHA will suffice? If so add it as a comment so we can do a verification at some point. |
Sorry for the delay here. It was a gutsy pull so fell on me. I am adding new keys to the pantry for this, thus this PR will have this applied (by me): vendor:
platforms:
- linux
- darwin
- windows
dependencies:
rust-lang.org/rustup: '*'
script:
- rustup default {{ version }}
- rustup component add rust-analyzer
# used for type checking in IDEs
- rustup component add rust-src
# copy installation
- mkdir -p {{ prefix }}
- cp -r ~/.rustup/toolchains/*/* {{prefix}}
# do cleanup
- rm -rf {{prefix}}/share/doc Our rules become:
We should port all existing vendored entries as possible. eg. for bun we vendored it because its build was too tedious. It may still be too tedious (it required its own custom zig compile 😬). Refs #843 |
k well I looked into how to do this with BrewKit and brewkit is too spaghetti. I need to rewrite BrewKit for several reasons so this will wait for then. For now let’s just comment out the build instructions I will add the commit. |
k well while fixing the conflict it appears at some point we fixed rust-src? I wish it was easier to read a single file’s history 😪. @jhheider you know the current state of rust bugs? |
I don't believe there's any open with us except the rust-analyzer/rust-src one. |
Initially meant to just add the src component so IDEs would work (pkgxdev/pkgx#685) but during the process of getting that installed (can only be installed via rustup) I found it easier to move the whole thing to be installed via rustup. This leads to faster build times on CI as well as the version not showing up as
x.x.x-dev
+ it should allow more versatility in choosing what components to install.This successfully fixes std definitions for me in IDEs (IntelliJ + VSCode) but does not yet lead to on the fly checks working on IntelliJ.. that will continue to be investigated. This PR already adds some benefit though and is a good first step.
Edit: it looks like it might fix more issues in the IDE (including support for expanding macros) more details in the linked issue.