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.

Sign up for GitHub

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

Improve arch and system autodetection #52 #57

Closed
wants to merge 1 commit into from
Closed

Improve arch and system autodetection #52 #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,16 @@ var BoldColor = color.New(color.OpBold).Render
var LoadingSpinner = spinner.New(spinner.CharSets[9], 100*time.Millisecond, spinner.WithColor("cyan"), spinner.WithHiddenCursor(true))

// RegexDarwin is a regular express for darwin systems
var RegexDarwin = `(?i)(darwin|mac(os)?|apple|osx)`
var RegexDarwin = `(?i)(darwin|mac(os)?|apple|osx|.dmg)`

// RegexWindows is a regular express for windows systems
var RegexWindows = `(?i)(windows|win)`
var RegexWindows = `(?i)(windows|win|.msi|.exe|.appx)`

Comment on lines -26 to 30
Copy link
Owner

Choose a reason for hiding this comment

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

Could you remove .dmg and .appx? Don't think these help because even if you can download them you want be able to properly install these formats through stew.

// RegexArm64 is a regular express for arm64 architectures
var RegexArm64 = `(?i)(arm64|aarch64)`
var RegexArm64 = `(?i)(arm64|aarch64|arm64e)`

// RegexAmd64 is a regular express for amd64 architectures
var RegexAmd64 = `(?i)(x86_64|amd64|x64)`
var RegexAmd64 = `(?i)(x86_64|amd64|x64|amd64e)`

// Regex386 is a regular express for 386 architectures
var Regex386 = `(?i)(i?386|x86_32|amd32|x32)`
Expand Down
8 changes: 5 additions & 3 deletions lib/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/json"
"fmt"
"regexp"
"strings"

"github.com/marwanhawari/stew/constants"
)
Expand Down Expand Up @@ -125,8 +124,10 @@ func assetsFound(releaseAssets []string, releaseTag string) error {

func filterReleaseAssets(assets []string) []string {
var filteredAssets []string
re := regexp.MustCompile(`\.(sha(256|512)(sum)?)$`)

for _, asset := range assets {
if strings.HasSuffix(asset, ".sha256") {
if re.MatchString(asset) {
Comment on lines +127 to +130
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this to the constants.go file? Include a comment like:

// RegexChecksum is a regular expression for matching checksum files
var RegexChecksum = `\.(sha(256|512)(sum)?)$`

continue
}
filteredAssets = append(filteredAssets, asset)
Expand All @@ -152,6 +153,7 @@ func DetectAsset(userOS string, userArch string, releaseAssets []string) (string
}

filteredReleaseAssets := filterReleaseAssets(releaseAssets)

for _, asset := range filteredReleaseAssets {
if reOS.MatchString(asset) {
detectedOSAssets = append(detectedOSAssets, asset)
Expand Down Expand Up @@ -189,7 +191,7 @@ func DetectAsset(userOS string, userArch string, releaseAssets []string) (string
}
}
if finalAsset == "" {
finalAsset, err = WarningPromptSelect("Could not automatically detect the release asset matching your OS/Arch. Please select it manually:", filteredReleaseAssets)
finalAsset, err = WarningPromptSelect("Could not automatically detect the release asset matching your OS/Arch. Please select it manually:", detectedFinalAssets)
Copy link
Owner

Choose a reason for hiding this comment

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

I think if filtering fails, I'd rather be conservative and show the user everything, so could you please revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

But can we add some logic around interactive and non-I reactive terminals? I use stew in docker installations and “I will show you everything” is not working well in this scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it can be an option, but everybody will forget it on a first run. Just like -y is always forgotten in “apt install package“ in scripts

Copy link
Owner

Choose a reason for hiding this comment

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

For non-interactive installs, you should be installing from a Stewfile.lock.json.

stew install Stewfile.lock.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh. It will not work this way. I'm on Mac, my container is linux and maybe on different architecture

Copy link
Owner

Choose a reason for hiding this comment

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

How does presenting users with a filtered list of options here help with non-interactive installs?

if err != nil {
return "", err
}
Expand Down