Change upstream via $RUSTUP_DIST_SERVER #521

knight42 · 2016-06-08T05:37:18Z

The downloads from the original https://static.rust-lang.org/ are rather slow in some regions, e.g. China. It is reasonable to enable users to choose other mirror sites.

Besides, I doubt that setting RUSTUP_DIST_ROOT is able to alter the root URL for downloading Rust toolchains and updates, since the urls written in the manifest files are still something like https://static.rust-lang.org/dist/2016-05-24/rust-1.9.0-aarch64-unknown-linux-gnu.tar.gz, which makes no difference.

brson · 2016-06-08T23:49:14Z

Thanks for the PR. This is an important consideration.

I'm a little worried about the proliferation of these env vars that modify the download. It seems to me that most people would expect setting RUSTUP_DIST_ROOT to do this kind of replacement of the manifest URLs. How would you feel about doing this same thing but using the dist_root value from DownloadCfg? Off-hand I can't think of any reason that would be undesirable.

cc @Diggsey @alexcrichton

Diggsey · 2016-06-09T00:21:28Z

I agree we should be using DIST_ROOT for this. It also might be worth making it a setting (like host triple), to reduce the need to set environment variables (while still allowing the environment variable to act as an override)

knight42 · 2016-06-09T09:09:56Z

@brson @Diggsey
At present, only toolchains and its updates could be downloaded from RUSTUP_DIST_ROOT, what if I want to download cargo, which is located at https://static.rust-lang.org/cargo-dist/?

Is it acceptable to change the value of RUSTUP_DIST_ROOT to 'https://static.rust-lang.org' and append the 'dist' in the program?

alexcrichton · 2016-06-09T10:49:45Z

I agree with @brson that this may be one too may be one too many config variables, I'd expect solutions here along the lines of:

When mirroring, the manifest is updated with altered URLs
Locally changing the resolution of static.rust-lang.org

knight42 · 2016-06-09T12:04:11Z

@alexcrichton
Option A breaks the consistency with upstream and option B requires extra work on client side. They both seem kind of inelegant.

IMO, this is a case that should be handled by rustup which is a "toolchain installer".

What do you think?

alexcrichton · 2016-06-09T15:18:42Z

In my mind the purpose of the manifest is just metadata for the distribution, so it's fine for that to get modified somewhat arbitrarily. (i.e. I'd probably still personally go towards that route)

Diggsey · 2016-06-09T15:42:00Z

It does make it more difficult to create a mirror though. Couldn't we just avoid the problem by using relative paths in the manifest files?

brson · 2016-06-10T01:46:40Z

At present, only toolchains and its updates could be downloaded from RUSTUP_DIST_ROOT, what if I want to download cargo, which is located at https://static.rust-lang.org/cargo-dist/?

This is a good point. The manifest already contains cargo-dist URLs. multirust used a variable called RUSTUP_DIST_SERVER (I think) that just contained the server name and not the dist directory. We could go back to that scheme (probably put RUSTUP_DIST_ROOT on the path to deprecation, which would allow the replacement to work as long as all the paths were on the same server.

It's worth noting that I expect there to eventually be functionality to merge external manifests into the manifest definition, so e.g. @steveklabnik can publish intermezzos bins that work with the normal toolchain. Those manifests would point to resources on a different servo. I consider that use case pretty niche though and don't anticipate problematic interactions with this URL-replacement feature.

brson · 2016-06-10T01:48:19Z

It does make it more difficult to create a mirror though. Couldn't we just avoid the problem by using relative paths in the manifest files?

Yes. Possibly. I recall that @alexcrichton and I discussed this and made the conscious decision to use full paths, but don't recall why. Using relative paths would have strange interactions with the above feature I mentioned where we might merge two different manifests from two different sources.

alexcrichton · 2016-06-10T09:40:08Z

I wouldn't mind switching to a different variable as the one source of configuration, I largely just wouldn't want to have quite a large myriad of ways to configure rustup.

knight42 · 2016-06-11T01:26:38Z

@brson

I have updated my code, replacing the urls with RUSTUP_DIST_SERVER.

knight42 · 2016-06-11T14:52:28Z

@Diggsey @alexcrichton @brson
Any further suggestion?

knight42 · 2016-06-13T03:22:43Z

Any progress？

brson · 2016-06-14T17:45:37Z

Thanks for the updates @knight42. I'm glad you left the backwards-compatibility code in. Good idea.

I'd prefer if the loading of RUSTUP_DIST_SERVER wasn't duplicated 3 times. Would it be possible instead to change the dist_root_url value in Config to dist_root_server, then get the two cases in Manifestation from the Config type? It would require making the compatibility code remove any trailing dist in the path.

Would you also change the use of RUSTUP_DIST_ROOT in the test suite to use this new value instead?

I'd still like to see what @Diggsey thinks of this solution since he created the RUSTUP_DIST_ROOT variable.

Diggsey · 2016-06-14T18:05:47Z

I'm for this: the deviation from rustup.sh was from the initial port to rust, when it seemed like everything was under the /dist folder, to make life a little easier for people wanting to set up a mirror.

Add public member `dist_server: String` For the convenience of modifying the urls written in manifests, since the update functions can only access `temp_cfg`. In addition, the constructor function of `temp_cfg` accepts an additional `dist_root_server` now.

Add two public members to `struct Cfg`: `dist_root_url: String` `dist_root_server: String` The reason I changed the type of `dist_root_url` from Cow<str> to String is that I have to modify the original str and using String seems a bit handier.

brson · 2016-06-15T06:53:09Z

src/rustup/config.rs

@@ -93,7 +107,8 @@ impl Cfg {
            gpg_key: gpg_key,
            notify_handler: notify_handler,
            env_override: env_override,
-            dist_root_url: dist_root_url,
+            dist_root_url: dist_root,


This patch looks great now, but is it possible to remove this dist_root_url field completely? Looks like it's mainly used by rustup_dist::dist, so that module can just be changed to append "/dist".

@brson I am afraid not.

The dist_root_url field is accessed in rustup::toolchain(see here), which is used to construct the DownloadCfg.

Because the type of DownloadCfg.dist_root is &str, it is impossible to assign a modified value.

knight42 · 2016-06-15T14:59:56Z

@brson
All tests passed! 😃

BTW, AppVeyor is goddamn slow 💔

Diggsey · 2016-06-15T15:03:52Z

@knight42 Yeah, free accounts have zero parallelism, so the builds have to run sequentially.

knight42 · 2016-06-16T04:38:59Z

Could this be merged now?

brson · 2016-06-17T12:06:33Z

Thanks so much @knight42!

crlf0710 · 2019-08-27T17:10:47Z

cc #1467

knight42 mentioned this pull request Jun 8, 2016

[Modify] rust-static: 更新toml 里的url指向本地 ustclug/mirrorrequest#32

Closed

knight42 force-pushed the change-static-site branch from c8723d5 to a4aa240 Compare June 11, 2016 01:19

knight42 changed the title ~~Change upstream via $RUST_STATIC_SITE~~ Change upstream via $RUSTUP_DIST_SERVER Jun 11, 2016

Knight added 6 commits June 15, 2016 07:17

Update README

8a3c442

mock::clitools: Use RUSTUP_DIST_SERVER instead

19e808c

rustup_dist::dist: Add const DEFAULT_DIST_SERVER

48252db

rustup_dist::manifestation: Modify urls in manifest

98b7286

knight42 force-pushed the change-static-site branch from c8e5a4b to 98b7286 Compare June 15, 2016 06:16

Make ci happy

9763334

knight42 force-pushed the change-static-site branch from 6ff67a2 to 9763334 Compare June 15, 2016 06:47

brson reviewed Jun 15, 2016
View reviewed changes

brson merged commit 5009823 into rust-lang:master Jun 17, 2016

knight42 deleted the change-static-site branch June 17, 2016 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change upstream via $RUSTUP_DIST_SERVER #521

Change upstream via $RUSTUP_DIST_SERVER #521

knight42 commented Jun 8, 2016

brson commented Jun 8, 2016

Diggsey commented Jun 9, 2016

knight42 commented Jun 9, 2016

alexcrichton commented Jun 9, 2016

knight42 commented Jun 9, 2016 •

edited

alexcrichton commented Jun 9, 2016

Diggsey commented Jun 9, 2016

brson commented Jun 10, 2016

brson commented Jun 10, 2016

alexcrichton commented Jun 10, 2016

knight42 commented Jun 11, 2016

knight42 commented Jun 11, 2016

knight42 commented Jun 13, 2016

brson commented Jun 14, 2016

Diggsey commented Jun 14, 2016

brson Jun 15, 2016

knight42 Jun 15, 2016

knight42 commented Jun 15, 2016

Diggsey commented Jun 15, 2016

knight42 commented Jun 16, 2016

brson commented Jun 17, 2016

crlf0710 commented Aug 27, 2019

Change upstream via $RUSTUP_DIST_SERVER #521

Change upstream via $RUSTUP_DIST_SERVER #521

Conversation

knight42 commented Jun 8, 2016

brson commented Jun 8, 2016

Diggsey commented Jun 9, 2016

knight42 commented Jun 9, 2016

alexcrichton commented Jun 9, 2016

knight42 commented Jun 9, 2016 • edited

alexcrichton commented Jun 9, 2016

Diggsey commented Jun 9, 2016

brson commented Jun 10, 2016

brson commented Jun 10, 2016

alexcrichton commented Jun 10, 2016

knight42 commented Jun 11, 2016

knight42 commented Jun 11, 2016

knight42 commented Jun 13, 2016

brson commented Jun 14, 2016

Diggsey commented Jun 14, 2016

brson Jun 15, 2016

Choose a reason for hiding this comment

knight42 Jun 15, 2016

Choose a reason for hiding this comment

knight42 commented Jun 15, 2016

Diggsey commented Jun 15, 2016

knight42 commented Jun 16, 2016

brson commented Jun 17, 2016

crlf0710 commented Aug 27, 2019

knight42 commented Jun 9, 2016 •

edited