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

Update Go version #24

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

FlowingSPDG
Copy link

@FlowingSPDG FlowingSPDG commented Sep 12, 2024

Hi, I tried to make a plugin by myself and noticed Go itself and several tools version are old so I updated.

Updated tools

  • Go 1.19 -> 1.23 (go mod tidy -go=1.23)
  • golangci-lint v1.50.0 -> v1.61.1
  • yaegi 0.14.2 -> v0.16.1

Updated tool settings

  • golangci-lint
    • Update deprecated/deactivated linters.
    • replace goerr133 to err133
    • replaced fmt.Errorf to errors.New

Other

Maybe we can use package plugindemo instead of package plugindemo_test for demo_test.go since golangci-lint detects depguard because it imports not-test package?

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Using the latest version of Go is not required.

The version inside the go.mod is the min Go version and only need to be updated when using specific elements of a Go version.

go.mod Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@ jobs:

strategy:
matrix:
go-version: [ 1.19, 1.x ]
go-version: [ 1.23, 1.x ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
go-version: [ 1.23, 1.x ]
go-version: [ oldstable, stable ]

Copy link
Author

@FlowingSPDG FlowingSPDG Sep 13, 2024

Choose a reason for hiding this comment

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

I noticed it does not work with actions/setup-go@v2.
updated with db60eba :)

edit: I noticed(again) that darwin platform does not support x32. Should we remove them? @ldez

.github/workflows/main.yml Outdated Show resolved Hide resolved
Apply suggestions

Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
@FlowingSPDG
Copy link
Author

@ldez
Hey, thanks for the review!

I applied your suggestion :) thank you!

go.mod Show resolved Hide resolved
@ldez
Copy link
Contributor

ldez commented Sep 13, 2024

All the following section:

    steps:
      # https://github.com/marketplace/actions/setup-go-environment
      - name: Set up Go ${{ matrix.go-version }} on ${{ matrix.os }}(${{ matrix.arch }})
        uses: actions/setup-go@v5
        with:
          go-version: ${{ matrix.go-version }}
          architecture: ${{ matrix.arch }}

      # https://github.com/marketplace/actions/checkout
      - name: Checkout code
        uses: actions/checkout@v2

      # https://github.com/marketplace/actions/cache
      - name: Cache Go modules
        uses: actions/cache@v3
        with:
          # In order:
          # * Module download cache
          # * Build cache (Linux)
          # * Build cache (Mac)
          # * Build cache (Windows)
          path: |
            ~/go/pkg/mod
            ~/.cache/go-build
            ~/Library/Caches/go-build
            %LocalAppData%\go-build
          key: ${{ runner.os }}-${{ matrix.arch }}-${{ matrix.go-version }}-go-${{ hashFiles('**/go.sum') }}
          restore-keys: |
            ${{ runner.os }}-${{ matrix.arch }}-${{ matrix.go-version }}-go-

should be replaced by:

steps:
  - uses: actions/checkout@v4
  - uses: actions/setup-go@v5
    with:
      go-version: ${{ matrix.go-version }}
      architecture: ${{ matrix.arch }}

@FlowingSPDG
Copy link
Author

@ldez Hi, sorry for late response since I was busy!

I updated the actions but got Bad CPU type in executable error on some job.
I'll dig into it and apply change if needed :)

@ldez
Copy link
Contributor

ldez commented Oct 10, 2024

Sorry you must remove:

architecture: ${{ matrix.arch }}

and the element related to the matrix.

Github action supports only amd64.

.github/workflows/go-cross.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
FlowingSPDG and others added 3 commits October 13, 2024 01:50
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
@FlowingSPDG
Copy link
Author

@ldez thank you for code suggestion, applied!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants