Skip to content
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

minimum viable fuelup #8

Merged
merged 20 commits into from
May 25, 2022
Merged

minimum viable fuelup #8

merged 20 commits into from
May 25, 2022

Conversation

eightfilms
Copy link
Contributor

@eightfilms eightfilms commented May 22, 2022

rough minimum viable fuelup that downloads latest core binaries to start building on Fuel Network and unpacks it into $HOME/.fuelup. Creating this PR up early for feedback and thoughts.

The following are installed through this script:

The forc-binaries tarball from the sway repo:
forc, forc-fmt, forc-explore, forc-explore

And fuel-core from fuel-core repo

Discussion:

I tackled the rustup codebase and found it too big to use its concepts to make an MVP right now. Was also looking at foundry for inspiration and realized they were using just bash scripts to download and install foundryup, in the end decided to do it in a clap app even though it would probably be much faster if we just script everything, but would probably be harder to introduce features later.

I think maybe profiles is something we can move towards after we have some sort of an MVP ready, with other basic features.

Included the bin (built for x86_64-apple-darwin) as well for now

Immediate things to do for this PR:

  • include a shell script named fuelup-init.sh(as rustup/foundry does) to download this fuelup bin, setup the system (create a /.fuelup dir for example) and run fuelup automatically, so devs would only need to do curl -L <url> | sh to be able to set up forc binaries + fuel-core together.

After this PR:

  • CI for fuelup, so we can release latest fuelup features/etc. as we iterate and provide more features for the toolchain.
  • fuelup default to set the default for certain things (not sure how this should work - should we be able to set different versions for each of forc, fuel-core, etc?)

cc: @mitchmindtree @JoshuaBatty @adlerjohn for feedback

@eightfilms eightfilms self-assigned this May 22, 2022
@adlerjohn adlerjohn added the enhancement enhancement to a current feature label May 22, 2022
@mitchmindtree
Copy link
Contributor

mitchmindtree commented May 23, 2022

Rust vs Bash

I tackled the rustup codebase and found it too big to use its concepts to make an MVP right now. Was also looking at foundry for inspiration and realized they were using just bash scripts to download and install foundryup, in the end decided to do it in a clap app even though it would probably be much faster if we just script everything, but would probably be harder to introduce features later.

I think using Rust and clap is the right choice here. Rust makes it much easier to enable cross-platform support, should we ever want to solve FuelLabs/sway#1526.

The more we lean on bash, the more restricted our options become w.r.t. native Windows support. If we want to support native Windows we would need a batch file equivalent for every bash file we have, no?

I realise we don't publish Windows binaries right now, but it's worth being conscious of these things in order to avoid making FuelLabs/sway#1526 even harder.

fuelup binary

Included the bin (built for x86_64-apple-darwin) as well for now

Rather than including the binaries as a part of this repo, perhaps we should publish the fuelup binary the same way we publish the forc binaries under the sway repo using CI? See here. Or is this mac binary just temporary for testing purposes?

Edit: Oh I see you've already got this under way at #9 :)

Toolchains

(not sure how this should work - should we be able to set different versions for each of forc, fuel-core, etc?)

Even though it's big, it would be worth looking into how rustup does this as it could be tricky to do this nicely in a cross-platform manner.

My initial intuition would be to do something like:

~/.fuelup/
    toolchains/
        default # symlink to user-selected default, e.g. -> stable
        nightly # symlink to latest nightly release directory, e.g. -> nightly-2022-05-23
        stable # symlink to latest versioned release directory, e.g. -> v0.13.0
        v0.13.0/
            - forc
            - forc-explore
            - forc-fmt
            - forc-lsp
            - fuel-core
        v0.12.0/
            <binaries>
        nightly-2022-05-23/
            <binaries>
        nightly-2022-05-22/
            <binaries>

This way there is a clear, unique location to which we clone each toolchain when we need them, and switching between toolchains is simply a matter of changing the symbolic link. Then I guess we just have to add ~/.fuelup/toolchains/default to PATH somehow? Or ask the user to? Don't quite remember how rustup does this.

If it turns out that symbolic links aren't the right choice (e.g. do they even work on Windows? I've never tried), then rather than symbolic links we could a tiny .rustup/toolchain/toolchain.toml file or something that points to the current stable, nightly, default and so on.

All that said, there are likely edge cases I'm not thinking of that might influence design, so would recommend trying to drill into rustup at least a bit to see if we're on the right track.

MVP

rough minimum viable fuelup that downloads latest core binaries

Fwiw I'd even be happy to just land a barebones/empty CLI app that hardly does anything just so that we can first get all the CI stuff going (just in case there are things with the init script or toolchain support that you're still unsure about or would like reviewed separately).

@eightfilms
Copy link
Contributor Author

Rust vs bash

Agreed on your points :) I thought about it for a long time and was considering pushing a shell script to serve as a starter fuelup but decided on just starting it off as a rustup and shipping it as a binary right away. Then we can use fuelup-init.sh for users to download the built version of rustup compatible with their system through the script (this is like how rustup-init works as well)

Fuelup bin

Or is this mac binary just temporary for testing purposes?

Yep, I checked it in for now so we can test it - once we have our first proper release with mac/linux compatibility we can remove the binary

Toolchains

Yeah, I believe what you described was accurate, think I saw a lot of symlink related code within rustup but I will look deeper into it and map it out for myself in a bit.

I think I was wondering more along the lines of how we might want to manage fuel-core together with forc, seeing as they aren't packaged together and released in 1 version like with packages like rust-clippy with rust? By the way, I included fuel-core in the installation because both are needed to start working with Sway immediately.

I guess rustup allows you to build your own custom toolchain with rustup toolchain link <toolchain> <path> that we can follow as well, then if the dev wants to package different versions of binaries together they would do it themselves?

@eightfilms
Copy link
Contributor Author

Also, should we keep the binaries within /.cargo/bin still? Or do we want to have our own bin path

@mitchmindtree
Copy link
Contributor

I think I was wondering more along the lines of how we might want to manage fuel-core together with forc, seeing as they aren't packaged together and released in 1 version like with packages like rust-clippy with rust?

Yeah this is less clear to me too. What happens in the rust + rust-clippy example you gave? How does rustup decide which version to fetch of each?

I guess rustup allows you to build your own custom toolchain with rustup toolchain link that we can follow as well, then if the dev wants to package different versions of binaries together they would do it themselves?

Yeah it might even be worth looking into solving this first, as it sounds like a more explicit case of handling linking for toolchains more generally. I'd imagine handling default, stable, nightly then might just fall out of this?

Also, should we keep the binaries within /.cargo/bin still? Or do we want to have our own bin path

Good question! We definitely don't want to edit or touch the user's .cargo/bin, and we also don't want to remove anything from the user's PATH.

We almost certainly want our own bin path. To ensure that users don't accidentally use a version of one of our binaries from ~/.cargo/bin, we'll want to have our bin location appear in PATH before ~/.cargo/bin does. Whether we want to edit the PATH ourselves or instruct the user to do so manually, I'll defer to following whatever rustup does.

@eightfilms eightfilms mentioned this pull request May 23, 2022
@eightfilms
Copy link
Contributor Author

eightfilms commented May 23, 2022

Ok - I have some additional thoughts on implementation of fuelup so I'm writing this based on what I've read and learnt about rustup so if someone knows more than I do or are more familiar than I am, please feel free to correct me:

rustup has the concept of Channels which basically makes a TOML file available on a dist server, here: https://static.rust-lang.org/dist/channel-rust-stable.toml

rustup depends on this TOML file to learn where/what version is appropriate for the desired channel (for example, since the above TOML file is for stable, all the versions there are considered stable)

This has also been a great resource for understanding rustup on a deeper level.

Ok, so for more relevant discussion to our current state of fuelup:

While rustup has useful concepts like channels and toolchains, I'm wondering if they are relevant to our current development state - we are still in early stages and iterating relatively quickly through our development on forc and fuel-core. We also do not yet develop on a train schedule where we have nightly, beta and stable releases on a regular schedule. If this is something we want to move towards, then we would have to decide on a schedule (I think the release cycle for Rust is six weeks, where a beta branch is promoted to be the new stable and then a new beta branch is created) but i think this would slow down development and I assume that we still want flexibility to be able to release versions of forc, fuel-core, etc. separately. If we do not want a schedule then we would have to decide on what constitutes as a stable release, for example. If that's the case what would constitute as a stable toolchain(?)

@mitchmindtree I think the layout you described above would only work if we defined what constitutes as a stable release for our core binaries, right? Eg. fuel-core does not actually share the same versioning as forc.

I think components and profiles make sense, for example I included downloading fuel-core as a binary in this startup script but technically it isn't really a requirement to start writing in Sway, so it should be treated as a component eventually instead (fuelup component add fuel-core, for example)

A minimal profile would just be forc and the std and core libs,
A default profile would include the above, + native plugins like forc-fmt, forc-lsp, forc-explore, and fuel-core.

@eightfilms
Copy link
Contributor Author

eightfilms commented May 23, 2022

Testing on this branch, latest commit:

# curl the script and pass 'install' subcommand
curl -L https://raw.githubusercontent.com/FuelLabs/fuelup/aa08ba4e9c4a13879b802b593c1018917da01da8/fuelup-init.sh | sh -s -- install

This installs the latest released versions of forc, forc-fmt, forc-lsp, forc-explore, and fuel-core into ~/.fuelup. I'm not quite sure this should be the final directory but perhaps we can store it within ~/.fuelup/bin, because there will be a need to store metadata related to fuelup to allow switching between versions

Results:

Downloading the Forc toolchain
Fetching binary from https://github.com/FuelLabs/sway/releases/download/v0.13.0/forc-binaries-darwin_amd64.tar.gz
Fetching binary from https://github.com/FuelLabs/fuel-core/releases/download/v0.7.0/fuel-core-0.7.0-x86_64-apple-darwin.tar.gz
Unpacking and moving forc to /Users/bh/.fuelup...
Unpacking and moving forc-explore to /Users/bh/.fuelup...
Unpacking and moving forc-lsp to /Users/bh/.fuelup...
Unpacking and moving forc-fmt to /Users/bh/.fuelup...
Unpacking and moving fuel-core to /Users/bh/.fuelup...

(this is on a x86-64 mac)

The script downloads the x86-64-darwin-apple bin but we should soon have CI up and running and we can update the script to point to the releases based on linux/mac

@eightfilms eightfilms mentioned this pull request May 23, 2022
@eightfilms eightfilms marked this pull request as ready for review May 23, 2022 15:35
@adlerjohn
Copy link
Contributor

IIRC rustup asks the user to modify their PATH. Foundry does not and just silently changed my ~/.bashrcinstead of asking and I didn't like that since I put these in ~/.profile instead of ~/.bashrc.

@eightfilms
Copy link
Contributor Author

IIRC rustup asks the user to modify their PATH. Foundry does not and just silently changed my ~/.bashrcinstead of asking and I didn't like that since I put these in ~/.profile instead of ~/.bashrc.

I think rustup modifies PATH as well no? https://github.com/rust-lang/rustup/blob/master/src/cli/self_update/unix.rs#L82

I think there are installation options you can set for it to not modify your PATH, but the default installation cmd line on rustup.rs has these options:

Current installation options:


   default host triple: x86_64-apple-darwin
     default toolchain: stable (default)
               profile: default
  modify PATH variable: yes

@eightfilms
Copy link
Contributor Author

Wait I think I missed the point of your comment, seems like your point is we should probably just have a line telling the user to add to their PATH instead of modifying it for them, in that case I agree

@eightfilms
Copy link
Contributor Author

Any chance we can merge this to master soon so I can test releases? :) cc: @mitchmindtree

On that note: is it alright to test releases in this repo or should we use a separate repo for that?

@Voxelot
Copy link
Member

Voxelot commented May 24, 2022

You can test in another private repo by copying this ci script and adding a beginning step of

      - name: Checkout sources
        uses: actions/checkout@v2
        with:
          repository: 'FuelLabs/fuelup'
          ref: 'binggh/mvp'

And then make as many releases as you want without polluting this repo.

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bunch of small tweaks, but otherwise happy to land this.

Also, rather than using println! maybe we should use tracing from the start?

src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated Show resolved Hide resolved
src/download.rs Outdated
Ok(new_data.len())
}
})
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be returning the error rather than unwrapping?

src/download.rs Outdated
}
})
.unwrap();
transfer.perform().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here ^

src/download.rs Outdated
data.extend_from_slice(new_data);
Ok(new_data.len())
})
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling?

src/download.rs Outdated
Ok(new_data.len())
})
.unwrap();
transfer.perform().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here ^

@eightfilms
Copy link
Contributor Author

Just a bunch of small tweaks, but otherwise happy to land this.

Also, rather than using println! maybe we should use tracing from the start?

the printlns are meant to display info to the end user, is tracing still appropriate here?

@mitchmindtree
Copy link
Contributor

Yes I think so - you can see some discussion around this for forc (which also displays info to the end user) here.

@@ -27,6 +28,9 @@ pub fn install() -> Result<()> {
GITHUB_API_REPOS_BASE_URL, FUEL_CORE_REPO, RELEASES_LATEST
))?;

info!("Fetching forc {}", &forc_release_latest_tag);
info!("Fetching fuel-core {}", &fuel_core_release_latest_tag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this to after forc is fetched?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, below the corresponding download_file_and_unpack() for each package? The idea of putting them there is to tell the user which version the installer is fetching - but in an error message i'm working on it would already report the tag of the tarball that it's fetching so it doesn't make a difference(?)

Example error output below:

Failed to download tarball and write to /Users/bh/.fuelup/fuel-core-0.7.1-x86_64-apple-darwin.tar.gz from https://github.com/FuelLabs/fuel-core/releases/download/v0.7.1/fuel-core-0.7.1-x86_64-apple-darwin.tar.gz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh my apologies, I thought these were meant to indicate when the fetching of each package begins, so I thought each of these should happen before their corresponding download begins.

Copy link
Contributor Author

@eightfilms eightfilms May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the info! messages right before where the download for each one should begin i.e. the info message for receiving forc's latest tag should come right before its download function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement enhancement to a current feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants