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

Extend triple target to everything distributed by forc #667

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

alfiedotwtf
Copy link
Contributor

Note: this is rebased on top of #666 👹

There are 2 types of downloads:

  1. forc-binaries, which is of the format "{os}_{arch}"
  2. Everything else, which is the standard "{arch}-{vendor}-{os}"

Currently, we only use the "{os}_{arch}" URL if the component is forc, however the forc-binaries tarball contains plugins and executables not defined in the forc component.

This PR extends the "{os}_{arch}" result to everything distributed from forc.

This fixes an issue found while investigating #654.

@alfiedotwtf alfiedotwtf added bug Something isn't working fuelup labels Oct 4, 2024
@alfiedotwtf alfiedotwtf requested a review from a team October 4, 2024 07:44
@alfiedotwtf alfiedotwtf self-assigned this Oct 4, 2024
@fuel-service-user
Copy link
Contributor

fuel-service-user commented Oct 4, 2024

LCOV of commit 274463d during CI #2086

Summary coverage rate:
  lines......: 85.7% (2457 of 2868 lines)
  functions..: 46.1% (382 of 829 functions)
  branches...: 63.0% (290 of 460 branches)

Files changed coverage rate: n/a

@alfiedotwtf alfiedotwtf marked this pull request as draft October 22, 2024 00:07
@alfiedotwtf alfiedotwtf changed the title Extend triple target to everything distributed by forc (#654) Extend triple target to everything distributed by forc Oct 27, 2024
@alfiedotwtf alfiedotwtf force-pushed the alfie/triple-double branch 2 times, most recently from 62341f0 to feb64b0 Compare October 28, 2024 05:12
@alfiedotwtf alfiedotwtf marked this pull request as ready for review October 28, 2024 05:26
@@ -53,40 +53,37 @@ impl TargetTriple {
}

pub fn from_component(component: &str) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

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

can we add a doc comment. feel free to tweak the below

/// Creates a target triple for the given component.
/// 
/// For components distributed by forc, uses the format "{os}_{architecture}".
/// For other components, uses the format "{architecture}-{vendor}-{os}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, done.

1. forc-binaries, which is of the format "{os}_{arch}"
2. Everything else, which is the standard "{arch}-{vendor}-{os}"

Currently, we only use the "{os}_{arch}" URL if the component is forc,
however the forc-binaries tarball contains plugins and executables not
defined in the forc component.

This PR extends the "{os}_{arch}" result to everything distributed from
forc.
Copy link
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Nice work!

@alfiedotwtf alfiedotwtf merged commit 003b485 into master Oct 29, 2024
17 checks passed
@alfiedotwtf alfiedotwtf deleted the alfie/triple-double branch October 29, 2024 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fuelup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants