download refactor + support single installations #62

eightfilms · 2022-06-03T15:31:55Z

Closes #60

Synopsis

This PR contains the following changes:

Refactor download.rs, change is mainly centered around DownloadCfg which is a representation of a configuration which allows you to specify a name and version to download a component.
Allows you to now specify names to install binaries and also at specific versions (examples below):

To install all (including fuelup at the end):

fuelup install

To install a specific component (latest version is fetched by default):

fuelup install forc

To install 2 different components:

fuelup install forc fuel-core

To install 2 different components, 1 with a different version:

fuelup install forc@0.14.5 fuel-core

You can specify versions by starting with 'v' as well:

fuelup install forc@v0.15.1

Some unhappy paths worth testing, starting with installing invalid components:

$ fuelup install forc-fmt
Error: Unrecognized component: forc-fmt

Wrong versioning (Note that these are also covered under tests):

$ fuelup install forc@0.15
Error: Invalid format for version: 0.15. Version must be in the format <major>.<minor>.<patch>

Note: to install a specific version the syntax strictly follows the format <name>@<version> by using regex parsing, where version follows the <major>.<minor>.<patch> notation

eightfilms · 2022-06-07T02:40:38Z

Testing this again and trying random commands to check for weird unhappy paths, currently this doesn't know how to handle:

fuelup install forc forc@0.14.5

Going to validate this by allowing only 1 of the same names to exist in a single command

mitchmindtree

Great stuff @binggh, looking really good! Just a couple small suggestions.

The forc@1.2.3 syntax looks nice too. Did rustup set precedent for that? Or did you come up with it?

mitchmindtree · 2022-06-08T06:05:00Z

src/commands/install.rs

+pub fn parse_component(component: &str) -> Result<(String, Option<String>)> {
+    if component.contains('@') {
+        let filtered = component.split('@').collect::<Vec<&str>>();
+        let semver_regex = Regex::new(r"^[v]?\d+\.\d+\.\d+$").unwrap();


Perhaps rather than introducing regex, we could use the semver crate? E.g. semver::Version::parse(version)? I'm thinking it might be a little more light-weight / easier to read for non-regex magicians.

you're right, this would definitely be alot more legible and would help with #64 as well (since we're checking versions), seems like it also handles validation which is everything we need for this

mitchmindtree · 2022-06-08T06:39:04Z

src/download.rs

+            release_url: match name {
+                "forc" => SWAY_RELEASE_DOWNLOAD_URL.to_string(),
+                "fuel-core" => FUEL_CORE_RELEASE_DOWNLOAD_URL.to_string(),
+                "fuelup" => FUELUP_RELEASE_DOWNLOAD_URL.to_string(),


I think it might be worth moving these component names into constants as they're used in quite a few places and it might help to avoid typos, e.g.

pub mod component { pub const FORC: &str = "forc"; pub const FUEL_CORE: &str = "fuel-core"; pub const FUELUP: &str = "fuelup"; } // ... match name { component::FORC => SWAY_RELEASE_DOWNLOAD_URL.to_string(), // etc

src/download.rs

eightfilms · 2022-06-08T07:47:08Z

The forc@1.2.3 syntax looks nice too. Did rustup set precedent for that? Or did you come up with it?

Nope - I'm not sure if rustup does this (don't think so) but I saw this syntax in cargo edit :p

eightfilms · 2022-06-08T09:01:45Z

I'm wondering if we should just store the version within DownloadCfg as a semver::Version as well instead of as a plain string 🤔

edit: shouldn't take too long to rewrite this so I shall

… string

eightfilms · 2022-06-08T09:40:30Z

Rewrote versioning to take semver::Version instead of a plain string. Turns out and feels like it is cleaner and more resilient to bugs than storing as a plain string. Thanks for the heads up on using the crate! @mitchmindtree

eightfilms · 2022-06-08T09:45:55Z

Testing this again and trying random commands to check for weird unhappy paths, currently this doesn't know how to handle:
fuelup install forc forc@0.14.5
Going to validate this by allowing only 1 of the same names to exist in a single command

Forgot to address this but I did update the behaviour by not executing the command at all if a duplicate input was given:

$ fuelup install forc@0.14.5 forc
Error: Invalid command due to duplicate input: forc

adlerjohn · 2022-06-08T16:09:32Z

$ rustup component add --help
...
OPTIONS:
        --target <target>          
        --toolchain <toolchain>    Toolchain name, such as 'stable', 'nightly', or '1.8.0'. For more information see
                                   `rustup help toolchain`

eightfilms · 2022-06-08T17:12:20Z

$ rustup component add --help
...
OPTIONS:
        --target <target>          
        --toolchain <toolchain>    Toolchain name, such as 'stable', 'nightly', or '1.8.0'. For more information see
                                   `rustup help toolchain`

Yup! The intention was always to go towards something like that (sorry I didn't make it clear) - working on supporting toolchains (latest and custom ones) will be the next step, but I think I had a rough start with the way i structured downloads, so I wanted to refactor that at least. Realized while refactoring that I could work also on single installations, and that helped me wrap my head around a better way to structure code here.

rustup component add adds a component to a toolchain, which we do not technically have yet... though I guess our current toolchain is already the latest, just not in name. I plan to kickoff supporting toolchains next by starting off with supporting latest first (#63, as well my comment here). Then, we can simply rename the commands to follow the rustup convention! It just didn't seem accurate to call it fuelup component add right now.

As for the toolchain-based experience that we'd like to pursue, since we do not have version sets bundling forc and fuel-core together (i'm working under the assumption that we should), that means if we want a toolchain-esque experience of rustup we have to think about how to tag compatible version of forc and fuel-core together, probably by making fuel-core follow forc's versioning (I think @mitchmindtree also mentioned this). Either that, or we have to think about fuelup from a components-first approach for now, where we let devs build custom toolchains (similar to rustup) based on what their project was built on and runs on.

What would, however, the developer experience look like for adding to custom toolchains?

For example rustup's method of custom toolchains seems to involve manually building and linking local builds, but we already have an answer here, which is to do fuelup component add forc@0.14.5 (an older forc, for example). So instead of manually building and linking components, I'm thinking if we can deviate from rustup here and maybe have an experience like:

fuelup toolchain new my-toolchain
fuelup default my-toolchain
fuelup component add forc@0.14.5 # adds forc v0.14.5 to my-toolchain

Apologies for this verbal vomit but I just had this stream of ideas that I had to puke out. Let me know if I'm mistaken about the approach we're going for here.

mitchmindtree · 2022-06-09T09:21:55Z

we have to think about how to tag compatible version of forc and fuel-core together, probably by making fuel-core follow forc's versioning (I think @mitchmindtree also mentioned this)

Just to clarify: I don't think fuel-core should be required to have the same version as forc as both have very distinct responsibilities. Rather, we should select a version for fuel-core based on the version selected for forc. I've opened a dedicated issue at #66.

eightfilms · 2022-06-09T09:25:40Z

we have to think about how to tag compatible version of forc and fuel-core together, probably by making fuel-core follow forc's versioning (I think @mitchmindtree also mentioned this)

Just to clarify: I don't think fuel-core should be required to have the same version as forc as both have very distinct responsibilities. Rather, we should select a version for fuel-core based on the version selected for forc. I've opened a dedicated issue at #66.

Yup ^ sorry, that's what I meant, selecting a compatible version of fuel-core based on forc.

mitchmindtree · 2022-06-10T03:57:12Z

rustup component add adds a component to a toolchain, which we do not technically have yet... though I guess our current toolchain is already the latest, just not in name. I plan to kickoff supporting toolchains next by starting off with supporting latest first (#63, as well my #46 (comment)). Then, we can simply rename the commands to follow the rustup convention! It just didn't seem accurate to call it fuelup component add right now.

Perhaps this is an indicator that we should define toolchains within fuelup first before moving ahead with this PR? It would be good to try and follow-suit with rustup as best we can for both familiarity and knowing that the approach is already well battle-tested.

As for the toolchain-based experience that we'd like to pursue, since we do not have version sets bundling forc and fuel-core together (i'm working under the assumption that we should), that means if we want a toolchain-esque experience of rustup we have to think about how to tag compatible version of forc and fuel-core together, probably by making fuel-core follow forc's versioning (I think @mitchmindtree also mentioned this). Either that, or we have to think about fuelup from a components-first approach for now, where we let devs build custom toolchains (similar to rustup) based on what their project was built on and runs on.

Hmmmmm 🤔

In retrospect, it's probably a wise choice from rustup to avoid encouraging the creation of custom toolchains from arbitrary sets of components in general. If not careful, it would be very easy for the responsibilities of rustup to veer from "Provides easy access to a set of compatible tools for Rust dev" to "Manages installation of all possible combinations of tools for Rust dev".

By allowing and encouraging users to build custom toolchains from arbitrary components with untested version combinations, I can foresee users frequently encountering strange compile/runtime errors as a result of incompatibilities between different sets of versions. We should likely only encourage usage of the set of toolchains we can actually test (i.e. stable, nightly, latest once they're defined).

If we take this "components-first" approach to toolchain building, I think we should avoid encouraging (or potentially even exposing) its usage, and emphasize that it is only really intended for use by developers of Sway, advanced use cases, and that for the most part it is an implementation detail of how fuelup forms its exposed toolchains.

Rather than letting the fact that we don't yet have well defined toolchains out of the box encourage a components-first approach, I think we should instead take this as a hint that we should prioritise defining our toolchains first (at least just latest for now), but the details on that should be discussed in #66.

In general, I'm much less sure that we should consider exposing custom toolchains as a feature unless we can think of a really good use-case that our stable, nightly and latest toolchains cannot satisfy. My intuition is that we're likely better off thinking of them as a potential burden and something to avoid if possible.

Feel free to let me know if you think I'm not considering something properly here @binggh! Curious to get your thoughts on this.

eightfilms · 2022-06-10T05:54:51Z

I think you raised up very good points that I didn't consider before so I stand corrected. Especially the point that supporting custom toolchains from the get go will probably create more problems for us than if we simply defined our toolchains first, and at least assure devs that these toolchains have been tested and let it be known that custom toolchains are experimental in nature and should only be for advanced users.

If this is the case then I should work on #63 first, and at least support this command: fuelup toolchain install latest (which is what we have now) along with the accompanying infra in the filesystem (storing the toolchain under .fuelup/toolchains/latest for example, and linking it to .fuelup/bin instead).

Then I think we have to come to an agreement on how #66 will work <- this is something that's not totally clear to me yet, and in hindsight, was actually the main reason why I was contemplating what I wrote above, which is that it's probably not an easy problem to define which fuel-core versions work in a stable manner with forc. But you're right in that it's not the solution to defer this problem to the dev, which was what I was missing in my line of thought.

With that said, I would still like to work from this PR if it's fine - it contains pretty useful refactoring to download.rs that is reusable to tackle #63

mitchmindtree

With that said, I would still like to work from this PR if it's fine - it contains pretty useful refactoring to download.rs that is reusable to tackle #63

Keeping the refactor sounds good! That said, I wonder if we should at least remove the publicly facing @<version> handling for now, or perhaps split it into a separate branch pending further consideration?

mitchmindtree · 2022-06-10T06:08:30Z

src/download.rs

    let handle = ureq::builder().user_agent("fuelup").build();
    let resp = handle.get(github_api_url).call()?;

    let mut data = Vec::new();
    resp.into_reader().read_to_end(&mut data)?;

    let response: LatestReleaseApiResponse = serde_json::from_str(&String::from_utf8_lossy(&data))?;
-    Ok(response.tag_name)
+
+    let version = Version::parse(&response.tag_name[1..response.tag_name.len()])?;


Suggested change

let version = Version::parse(&response.tag_name[1..response.tag_name.len()])?;

// Given a semver version with preceding 'v' (e.g. `v1.2.3`), take the slice after 'v' (e.g. `1.2.3`).

let version_str = &response.tag_name['v'.len()..];

let version = Version::parse(version_str)?;

Yep, this sounds good, i'll remove all the changes related to specific versioning from this PR for now, and we'll just merge changes related to the refactor

eightfilms · 2022-06-10T07:38:11Z

Removed version parsing from the install command - shouldn't be able to do fuelup install forc@0.14.5 anymore for now. You can still do:

fuelup install to install all components,
fuelup install forc|fuel-core|fuelup to install specific components

The command name probably needs changing eg. it should be fuelup toolchain install latest (to install everything), but will handle that when I'm tackling #63!

mitchmindtree · 2022-06-10T07:48:52Z

src/commands/install.rs

 };

 #[derive(Debug, Parser)]
-pub struct InstallCommand {}
+#[clap(override_usage = "\
+    fuelup install <COMPONENT>[@<VERSION>] ...")]


Woops I think we want to remove this override too?

Thanks for the catch!

eightfilms · 2022-06-14T05:50:58Z

Because this has deviated from #73 alot - will close this and instead merge to master from #73

binggh added 3 commits June 3, 2022 17:35

single installations prelim implementation

aee3e57

Refactor

c731aac

Remove print

f952e8f

eightfilms added the enhancement enhancement to a current feature label Jun 3, 2022

eightfilms self-assigned this Jun 3, 2022

binggh added 4 commits June 6, 2022 10:32

Merge branch 'master' into binggh/single-installations

5a2899a

Finish refactor

d1963e3

make clippy happy

1b330ca

Parse component inputs in the format <name>@<version>

54e84f5

eightfilms mentioned this pull request Jun 6, 2022

fuelup option to add to path #61

Merged

binggh added 4 commits June 7, 2022 09:41

Use DownloadCfg as input directly instead of name, version tuple

50298a9

Format version string with 'v'

b689c0f

Fix Cargo.toml dep ordering

f1eb93c

Accept 'v' infront of versions

7e08b0d

eightfilms requested a review from mitchmindtree June 7, 2022 02:05

eightfilms marked this pull request as ready for review June 7, 2022 02:07

Remove unused takes_value

af7e3d1

binggh added 6 commits June 7, 2022 10:41

Missing downloading message

1a64316

Handle case where components are mentioned twice

b9cca50

Add test case for duplicate inputs

031f656

fix: better wording for error

6edc43a

Fix wrong command in help message

1212c43

add FUELUP_REPO constant

15a4fb2

eightfilms mentioned this pull request Jun 7, 2022

fuelup check #64

Merged

binggh added 5 commits June 7, 2022 23:43

Use POSSIBLE_COMPONENTS in download

769540f

rename variable names -> component for consistency

361e9f3

missed out one name -> component change

22f5eb0

Reorder install all step so it fails if not all configs are valid

e5e36da

Clearer message for failing to get tag

731badd

mitchmindtree suggested changes Jun 8, 2022

View reviewed changes

binggh added 4 commits June 8, 2022 15:49

fix fuel-core's architecture matching

c23a9f9

Introduce component constants to reduce code dups

9a3635a

Use semver over regex

e4a7b8a

More sensible test examples

4a8d1c6

rewrote versioning in DownloadCfg as semver::Version instead of plain…

f8c88ec

… string

eightfilms requested a review from mitchmindtree June 8, 2022 09:41

mitchmindtree reviewed Jun 10, 2022

View reviewed changes

binggh added 3 commits June 10, 2022 15:31

Remove support for targeted version installations

ea33c28

better version of version parsing

c3d9c84

clippy

a51cc5c

eightfilms requested a review from mitchmindtree June 10, 2022 07:40

mitchmindtree reviewed Jun 10, 2022

View reviewed changes

Remove override usage

ab24294

mitchmindtree approved these changes Jun 10, 2022

View reviewed changes

eightfilms closed this Jun 14, 2022

eightfilms mentioned this pull request Jun 21, 2022

Refactor download.rs #60

Closed

eightfilms deleted the binggh/single-installations branch June 23, 2022 06:25

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

download refactor + support single installations #62

download refactor + support single installations #62

eightfilms commented Jun 3, 2022 •

edited

Loading

eightfilms commented Jun 7, 2022

mitchmindtree left a comment

mitchmindtree Jun 8, 2022

eightfilms Jun 8, 2022

mitchmindtree Jun 8, 2022

eightfilms commented Jun 8, 2022

eightfilms commented Jun 8, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

adlerjohn commented Jun 8, 2022

eightfilms commented Jun 8, 2022 •

edited

Loading

mitchmindtree commented Jun 9, 2022 •

edited

Loading

eightfilms commented Jun 9, 2022

mitchmindtree commented Jun 10, 2022

eightfilms commented Jun 10, 2022 •

edited

Loading

mitchmindtree left a comment

mitchmindtree Jun 10, 2022

eightfilms Jun 10, 2022

eightfilms commented Jun 10, 2022 •

edited

Loading

mitchmindtree Jun 10, 2022

eightfilms Jun 10, 2022

eightfilms commented Jun 14, 2022

-    let version = Version::parse(&response.tag_name[1..response.tag_name.len()])?;
+    // Given a semver version with preceding 'v' (e.g. `v1.2.3`), take the slice after 'v' (e.g. `1.2.3`).
+    let version_str = &response.tag_name['v'.len()..];
+    let version = Version::parse(version_str)?;

download refactor + support single installations #62

download refactor + support single installations #62

Conversation

eightfilms commented Jun 3, 2022 • edited Loading

Synopsis

eightfilms commented Jun 7, 2022

mitchmindtree left a comment

Choose a reason for hiding this comment

mitchmindtree Jun 8, 2022

Choose a reason for hiding this comment

eightfilms Jun 8, 2022

Choose a reason for hiding this comment

mitchmindtree Jun 8, 2022

Choose a reason for hiding this comment

eightfilms commented Jun 8, 2022

eightfilms commented Jun 8, 2022 • edited Loading

eightfilms commented Jun 8, 2022 • edited Loading

eightfilms commented Jun 8, 2022 • edited Loading

adlerjohn commented Jun 8, 2022

eightfilms commented Jun 8, 2022 • edited Loading

mitchmindtree commented Jun 9, 2022 • edited Loading

eightfilms commented Jun 9, 2022

mitchmindtree commented Jun 10, 2022

eightfilms commented Jun 10, 2022 • edited Loading

mitchmindtree left a comment

Choose a reason for hiding this comment

mitchmindtree Jun 10, 2022

Choose a reason for hiding this comment

eightfilms Jun 10, 2022

Choose a reason for hiding this comment

eightfilms commented Jun 10, 2022 • edited Loading

mitchmindtree Jun 10, 2022

Choose a reason for hiding this comment

eightfilms Jun 10, 2022

Choose a reason for hiding this comment

eightfilms commented Jun 14, 2022

eightfilms commented Jun 3, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

eightfilms commented Jun 8, 2022 •

edited

Loading

mitchmindtree commented Jun 9, 2022 •

edited

Loading

eightfilms commented Jun 10, 2022 •

edited

Loading

eightfilms commented Jun 10, 2022 •

edited

Loading