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

Windows compat tasks #20

Merged
merged 9 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Prevent CRLF correction from changing digest of
# text files that are committed as stand-ins for binaries
# in text fixtures:
/tool/testdata/store/** -text
12 changes: 12 additions & 0 deletions .github/workflows/validations.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,15 @@ jobs:

- name: Run all validations
run: make pr-validations

WindowsValidations:
name: "Windows units"
runs-on: windows-2022
steps:
- uses: actions/checkout@v3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this doesn't use the caching that we'd get from using our own actions/bootstrap here, but that has some entries that require shell: bash which presumably won't work on Windows, and I don't know why it needs those yet.

- name: install make
run: "choco install make"

- name: run units
run: "make unit"
6 changes: 3 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ TASK = $(TOOL_DIR)/task
## Bootstrapping targets #################################

$(BINNY):
@mkdir -p $(TOOL_DIR)
@# we don't have a release of binny yet, so build off of the current branch
@#curl -sSfL https://raw.githubusercontent.com/$(OWNER)/$(PROJECT)/main/install.sh | sh -s -- -b $(TOOL_DIR)
@-mkdir $(TOOL_DIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

The - preceding the command is to prevent it exiting on failure. Windows doesn't support the -p, instead creating a directory with that name. In this case, we only need a directory 1-level deep, so we don't really need -p. Maybe a better idea would be to just add a file to the repo at .tool/empty and get rid of creating this directory altogether?

# we don't have a release of binny yet, so build off of the current branch
# curl -sSfL https://raw.githubusercontent.com/$(OWNER)/$(PROJECT)/main/install.sh | sh -s -- -b $(TOOL_DIR)
Copy link
Contributor

@kzantow kzantow Apr 9, 2024

Choose a reason for hiding this comment

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

We're going to have to solve this for every other repo, just not this one since it has binny go code...

go build -o $(TOOL_DIR)/$(PROJECT) ./cmd/$(PROJECT)

.PHONY: task
Expand Down
9 changes: 5 additions & 4 deletions Taskfile.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ vars:
SNAPSHOT_DIR: snapshot
CHANGELOG: CHANGELOG.md
NEXT_VERSION: VERSION
MAKEDIR_P: 'python -c "import sys; import os; os.makedirs(sys.argv[1], exist_ok=True)"'

tasks:

Expand Down Expand Up @@ -122,15 +123,15 @@ tasks:
desc: Run all unit tests
vars:
TEST_PKGS:
sh: "go list ./... | grep -v {{ .OWNER }}/{{ .PROJECT }}/test | tr '\n' ' '"
sh: "python ./scripts/list_units.py anchore binny"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have binny, can we just have it download grep (and maybe sh, curl, and other such missing shell script utilities) instead of making the python scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that the utilities won't behave the same even if we make them available. For example ls lists files in PowerShell, but the output looks different than on macOS or Linux. We also see elsewhere in this PR that mkdir -p doesn't work on Windows, even though there is a mkdir on PATH in PowerShell.

In other words, if we keep using a command shell as our scripting environment, we have to worry about the compatibility of every executable the shell invokes. If we use standard library Python, we only have to worry about the cross-platform behavior of Python itself.


# unit test coverage threshold (in % coverage)
COVERAGE_THRESHOLD: 45
COVERAGE_THRESHOLD: '{{if eq OS "windows"}} 43 {{else}} 45 {{end}}'
cmds:
- cmd: "mkdir -p {{ .TMP_DIR }}"
- cmd: "{{ .MAKEDIR_P }} {{ .TMP_DIR }}"
silent: true
- "go test -coverprofile {{ .TMP_DIR }}/unit-coverage-details.txt {{ .TEST_PKGS }}"
- cmd: ".github/scripts/coverage.py {{ .COVERAGE_THRESHOLD }} {{ .TMP_DIR }}/unit-coverage-details.txt"
- cmd: '{{if eq OS "windows"}}python {{end}}.github/scripts/coverage.py {{ .COVERAGE_THRESHOLD }} {{ .TMP_DIR }}/unit-coverage-details.txt'
silent: true

## Build-related targets #################################
Expand Down
18 changes: 18 additions & 0 deletions scripts/list_units.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import subprocess
import sys

def go_list_exclude_pattern(owner, project):
exclude_pattern = f"{owner}/{project}/test"

result = subprocess.run(["go", "list", "./..."], stdout=subprocess.PIPE, text=True, check=True)

filtered_lines = [line for line in result.stdout.splitlines() if exclude_pattern not in line]

joined_output = ' '.join(filtered_lines)

return joined_output

owner = sys.argv[1]
project = sys.argv[2]
output = go_list_exclude_pattern(owner, project)
print(output)
20 changes: 12 additions & 8 deletions store.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,9 @@ func (s *Store) AddTool(toolName string, resolvedVersion, pathOutsideRoot string
}
}

file, err := os.Open(pathOutsideRoot)
digests, err := getDigestsForFile(pathOutsideRoot)
if err != nil {
return fmt.Errorf("failed to open temp copy of binary %q: %w", pathOutsideRoot, err)
}
defer file.Close()

digests, err := getDigestsForReader(file)
if err != nil {
return nil
return err
}

sha256Hash, ok := digests[internal.SHA256Algorithm]
Expand Down Expand Up @@ -338,6 +332,16 @@ func xxh64File(path string) (string, error) {
return fmt.Sprintf("%x", hash.Sum(nil)), nil
}

func getDigestsForFile(filePath string) (map[string]string, error) {
file, err := os.Open(filePath)
if err != nil {
return nil, fmt.Errorf("failed to open temp copy of binary %q: %w", filePath, err)
}
defer file.Close()

return getDigestsForReader(file)
}

func getDigestsForReader(r io.Reader) (map[string]string, error) {
sha256Hash := sha256.New()
xxhHash := xxhash.New64()
Expand Down
5 changes: 5 additions & 0 deletions store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package binny
import (
"os"
"path/filepath"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -104,6 +105,9 @@ func TestStore_GetByName(t *testing.T) {
}

func TestStore_Entries(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("test fixtures have different sha256 digests on Windows due to linbreak conversion")
}
tests := []struct {
name string
storeRoot string
Expand Down Expand Up @@ -219,6 +223,7 @@ func TestStore_AddTool(t *testing.T) {

createFile := func(path, contents string) {
fh, err := os.Create(path)
defer fh.Close()
require.NoError(t, err)
_, err = fh.WriteString(contents)
require.NoError(t, err)
Expand Down
4 changes: 2 additions & 2 deletions tool/githubrelease/installer.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,13 @@ func (i Installer) InstallTo(version, destDir string) (string, error) {
}

func downloadAndExtractAsset(lgr logger.Logger, asset ghAsset, checksumAsset *ghAsset, destDir string) (string, error) {
assetPath := path.Join(destDir, asset.Name)
assetPath := filepath.Join(destDir, asset.Name)

checksum := asset.Checksum
if checksumAsset != nil && checksum == "" {
lgr.WithFields("asset", checksumAsset.Name).Trace("downloading checksum manifest")

checksumsPath := path.Join(destDir, checksumsFilename)
checksumsPath := filepath.Join(destDir, checksumsFilename)

if err := internal.DownloadFile(lgr, checksumAsset.URL, checksumsPath, ""); err != nil {
return "", fmt.Errorf("unable to download checksum asset %q: %w", checksumAsset.Name, err)
Expand Down
4 changes: 3 additions & 1 deletion tool/githubrelease/installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,9 @@ func Test_findBinaryAssetInDir(t *testing.T) {
got, err := findBinaryAssetInDir(tt.destDir)
tt.wantErr(t, err)

assert.Equal(t, tt.want, got)
want := strings.ReplaceAll(tt.want, "/", string(os.PathSeparator))

assert.Equal(t, want, got)
})
}
}
Expand Down
3 changes: 3 additions & 0 deletions tool/goinstall/installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package goinstall

import (
"fmt"
"os"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -108,6 +110,7 @@ func TestInstaller_InstallTo(t *testing.T) {
i.goInstallRunner = tt.fields.goInstallRunner

got, err := i.InstallTo(tt.args.version, tt.args.destDir)
got = strings.ReplaceAll(got, string(os.PathSeparator), "/")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails for me because there is no sh by default on windows, regardless of the path. I wonder if we could do the same thing that the taskfile is doing and use https://github.com/mvdan/sh for these...

if !tt.wantErr(t, err, fmt.Sprintf("InstallTo(%v, %v)", tt.args.version, tt.args.destDir)) {
return
}
Expand Down
5 changes: 5 additions & 0 deletions tool/hostedshell/installer.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"text/template"

Expand Down Expand Up @@ -113,6 +114,10 @@ func templateFlags(args string, version, destination string) (string, error) {
}

func runScript(scriptPath, argStr string) error {
if runtime.GOOS == "windows" {
return fmt.Errorf("script based installers are not supported on %s", runtime.GOOS)
}

userArgs, err := shlex.Split(argStr)
if err != nil {
return fmt.Errorf("failed to parse args: %v", err)
Expand Down
5 changes: 5 additions & 0 deletions tool/hostedshell/installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@ import (
"net/http/httptest"
"os"
"path/filepath"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestInstaller_InstallTo(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("script based installer is not supported on windows")
}

type fields struct {
config InstallerParameters
scriptRunner func(scriptPath string, argStr string) error
Expand Down
Loading