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

ci: add path to go.sum to actions/setup-go #155

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Sep 21, 2024

I just noticed that actions/setup-go complains about the missing
go.sum file:

Restore cache failed: Dependencies file is not found in /home/runner/work/sys/sys. Supported file pattern: go.sum

Apparently this happens because of two reasons:

  1. actions/checkout should be run _before actions/setup-go.

  2. There's no top-level go.sum file.

The first problem is easy to fix.

As for the second one, documentation1 suggests using a wild card in
such cases, but using neither "*/go.sum" nor "**/go.sum" works, as not
all modules have go.sum, and so it fails with the following error:

Restore cache failed: Some specified paths were not resolved, unable to cache dependencies.

Alas, we have to add an extra step to list the available go.sum files.
The alternative would be listing them all, which is maintainers' nightmare.

(The contents of these files are used as an input when calculating the
cache checksum, essentially meaning if any of these files are changed,
the cache will be invalidated.)

@kolyshkin kolyshkin force-pushed the gha-go-cache branch 15 times, most recently from b86edba to 649e3be Compare September 21, 2024 04:37
@kolyshkin kolyshkin marked this pull request as ready for review September 21, 2024 04:37
I just noticed that actions/setup-go complains about the missing
go.sum file:

>  Restore cache failed: Dependencies file is not found in /home/runner/work/sys/sys. Supported file pattern: go.sum

Apparently this happens because of two reasons:

 1. actions/checkout should be run _before actions/setup-go.

 2. There's no top-level go.sum file.

The first problem is easy to fix.

As for the second one, documentation[1] suggests using a wild card in
such cases, but using neither "*/go.sum" nor "**/go.sum" works, as not
all modules have go.sum, and so it fails with the following error:

> Restore cache failed: Some specified paths were not resolved, unable to cache dependencies.

Alas, we have to add an extra step to list the available go.sum files.
The alternative would be listing them all, which is maintainers' nightmare.

(The contents of these files are used as an input when calculating the
cache checksum, essentially meaning if any of these files are changed,
the cache will be invalidated.)

[1]: https://github.com/actions/setup-go/blob/main/README.md#caching-dependency-files-and-build-outputs

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Collaborator Author

Rebased; description updated; PTAL @tianon @thaJeztah

@thaJeztah
Copy link
Member

I'm somewhat wondering if it's worth the extra complexity, given that it's a pretty small repository, and most tests should run in a short amount of time, even if no caching 🤔

@kolyshkin
Copy link
Collaborator Author

I'm somewhat wondering if it's worth the extra complexity, given that it's a pretty small repository, and most tests should run in a short amount of time, even if no caching 🤔

I agree caching won't help much here (perhaps if we add more modules and/or dependencies it will help in the future, but not now for sure).

The only immediate benefits I can think of are:

  1. no bad messages in test logs (pure aesthetics);
  2. caching allows to skip downloading some dependencies from github and therefore will result in less flakes from CI (at times go mod download which is part of make lint fails).

Since 1 can be solved by disabling the cache, and 2 is very rare, here is an alternative PR: #160

@thaJeztah
Copy link
Member

Yes, I think within context of this repository, I prefer the simpler solution. For other repositories, perhaps a more extended solution could make sense, but I think for this one we should be fine with (trying to) keep it simple.

Still hoping one day these situations will become easier, and go's tooling to provide better ways for this that don't require all these workaround through makefiles etc.

@tianon
Copy link
Member

tianon commented Sep 27, 2024

I tried to squash the same warning in my own code in docker-library/bashbrew#102 and ended up giving up (although the situation there is slightly different -- it's an action trying to use actions/setup-go as a sub-action). 😭

Having **/go.sum not work strikes me as a bug in the globbing implementation of https://github.com/actions/setup-go but I'm having trouble finding useful matches for it in the upstream issues. 🤔

@kolyshkin
Copy link
Collaborator Author

I tried to squash the same warning in my own code in docker-library/bashbrew#102 and ended up giving up (although the situation there is slightly different -- it's an action trying to use actions/setup-go as a sub-action). 😭

Having **/go.sum not work strikes me as a bug in the globbing implementation of https://github.com/actions/setup-go but I'm having trouble finding useful matches for it in the upstream issues. 🤔

It took me a while to write the patch in this PR that actually works, mostly due to two things:

  1. Windows (using powershell by default).
  2. Unclear documentation on GHA steps' outputs, especially multi-line ones.

I agree that wildcard should just work here, but I'm surprised there's not even an issue opened. Maybe I will open one a tad later.

@kolyshkin
Copy link
Collaborator Author

Closing in favor of #160.

@kolyshkin kolyshkin closed this Sep 27, 2024
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.

3 participants