fix: windows compilation #1101

Merged

earl-warren merged 6 commits from :fix-windows-compilation into main 2025-10-24 07:00:43 +00:00

Conversation 13 Commits 6 Files changed 3 +24 -2

volkmnv commented 2025-10-20 12:01:13 +00:00

Contributor

• Copy link
• Copy Markdown

Original pull request: #987
Fixes #1032

• bug fixes
  • PR: fix: windows compilation

Original pull request: https://code.forgejo.org/forgejo/runner/pulls/987 Fixes https://code.forgejo.org/forgejo/runner/issues/1032 <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - bug fixes - [PR](https://code.forgejo.org/forgejo/runner/pulls/1101): <!--number 1101 --><!--line 0 --><!--description Zml4OiB3aW5kb3dzIGNvbXBpbGF0aW9u-->fix: windows compilation<!--description--> <!--end release-notes-assistant-->

volkmnv added 1 commit 2025-10-20 12:01:13 +00:00

Moved suicide code out to platform specific files 080b8ecaff

volkmnv added 1 commit 2025-10-20 12:02:26 +00:00

Merge branch 'main' into fix-windows-compilation 384188353d

earl-warren commented 2025-10-20 12:07:56 +00:00

Contributor

• Copy link
• Copy Markdown

thanks for picking it up 😁

before I suggest things, how familiar are you with Go?

thanks for picking it up 😁 before I suggest things, how familiar are you with Go?

volkmnv commented 2025-10-20 12:11:07 +00:00

Author

Contributor

• Copy link
• Copy Markdown

just enough to compile a project, but I'm ready to learn

just enough to compile a project, but I'm ready to learn

volkmnv commented 2025-10-20 12:19:10 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I read your comments from the original pull request, but unfortunately, I only just now looked at util.go. I'll try to adjust the code accordingly.

I read your comments from the original pull request, but unfortunately, I only just now looked at util.go. I'll try to adjust the code accordingly.

volkmnv commented 2025-10-20 12:35:03 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I'm stuck at the same point as AnsisMalins. Since build tags seem to be file level only, I don't know how to handle this without creating a new utils.go in artifactscache containing only the suicide function. otherwise I have to duplicate all non platform specific code of handler.go to handler_windows.go. and as I understand it you said no to both options.

I'm stuck at the same point as AnsisMalins. Since build tags seem to be file level only, I don't know how to handle this without creating a new utils.go in artifactscache containing only the suicide function. otherwise I have to duplicate all non platform specific code of handler.go to handler_windows.go. and as I understand it you said no to both options.

proton-ab commented 2025-10-20 21:17:21 +00:00

First-time contributor

• Copy link
• Copy Markdown

The way I understand @earl-warren comment is that they want to end with this situation:

act/artifactcache/util.go

//go:build !windows

package artifactcache

import "syscall"

func suicide() error {
	return syscall.Kill(syscall.Getpid(), syscall.SIGTERM)
}

act/artifactcache/util_windows.go

package artifactcache

import "syscall"

func suicide() error {
	handle, err := syscall.GetCurrentProcess()
	if err != nil {
		return err
	}

	return syscall.TerminateProcess(handle, uint32(syscall.SIGTERM))
}

The way I understand @earl-warren comment is that they want to end with this situation: `act/artifactcache/util.go` ```go //go:build !windows package artifactcache import "syscall" func suicide() error { return syscall.Kill(syscall.Getpid(), syscall.SIGTERM) } ``` `act/artifactcache/util_windows.go` ```go package artifactcache import "syscall" func suicide() error { handle, err := syscall.GetCurrentProcess() if err != nil { return err } return syscall.TerminateProcess(handle, uint32(syscall.SIGTERM)) } ```

👍 1

proton-ab commented 2025-10-20 21:39:01 +00:00

First-time contributor

• Copy link
• Copy Markdown

That being said, I have some questions regarding original design here: Is the intention really to kill entire runner when cache servers encounters fatal error? Somehow that feels a bit extreme, though, as we are failing hard on conditions such as unable to read cache.

Imagine situation where artifact cache is mounted to NFS volume that is temporarily unavailable - I wouldn't want it to kill runner, just gracefully handle the fact that cache doesn't exist and pretend we didn't get hit instead, while logging error for operator.

That being said, I have some questions regarding original design here: Is the intention really to kill entire runner when cache servers encounters fatal error? Somehow that feels a bit extreme, though, as we are failing hard on conditions such as unable to read cache. Imagine situation where artifact cache is mounted to NFS volume that is temporarily unavailable - I wouldn't want it to kill runner, just gracefully handle the fact that cache doesn't exist and pretend we didn't get hit instead, while logging error for operator.

earl-warren commented 2025-10-20 21:46:06 +00:00

Contributor

• Copy link
• Copy Markdown

@proton-ab wrote in #1101 (comment):

The way I understand @earl-warren comment is that they want to end with this situation:

Exactly!

The policy is to failing hard in case an error is not recoverable, i.e. there is no way to ensure the runner will keep working as it should. If you think it could be done differently could you please open a feature request or a discussion? This a design question that has a wider scope than this pull request.

@proton-ab wrote in https://code.forgejo.org/forgejo/runner/pulls/1101#issuecomment-64145: > The way I understand @earl-warren comment is that they want to end with this situation: Exactly! The policy is to failing hard in case an error is not recoverable, i.e. there is no way to ensure the runner will keep working as it should. If you think it could be done differently could you please open a feature request or a discussion? This a design question that has a wider scope than this pull request.

volkmnv added 1 commit 2025-10-21 06:13:38 +00:00

Move suicide function to utils.go 0c26c71e4f

volkmnv commented 2025-10-21 06:14:09 +00:00

Author

Contributor

• Copy link
• Copy Markdown

Alright, then I misunderstood the comment in the original pull request.
Code has been changed as requested.

Alright, then I misunderstood the comment in the original pull request. Code has been changed as requested.

volkmnv added 1 commit 2025-10-21 06:15:51 +00:00

Merge branch 'main' into fix-windows-compilation 88b770c5ce

earl-warren requested changes 2025-10-21 06:36:14 +00:00

Dismissed

act/artifactcache/utils_windows.go Outdated

Show resolved Hide resolved

earl-warren commented 2025-10-21 06:36:11 +00:00

Contributor

• Copy link
• Copy Markdown

//go:build windows

otherwise it will have suicide() defined twice

`//go:build windows` otherwise it will have suicide() defined twice

volkmnv commented 2025-10-21 06:53:38 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I just tested it and it's defined twice with 'windows'

# code.forgejo.org/forgejo/runner/v11/act/artifactcache
act\artifactcache\utils_windows.go:5:6: suicide redeclared in this block
        act\artifactcache\utils.go:7:6: other declaration of suicide
act\artifactcache\utils.go:8:17: undefined: syscall.Kill

I just tested it and it's defined twice with 'windows' ``` # code.forgejo.org/forgejo/runner/v11/act/artifactcache act\artifactcache\utils_windows.go:5:6: suicide redeclared in this block act\artifactcache\utils.go:7:6: other declaration of suicide act\artifactcache\utils.go:8:17: undefined: syscall.Kill ```

volkmnv commented 2025-10-21 06:54:55 +00:00

Author

Contributor

• Copy link
• Copy Markdown

sorry missed the file name. but in container utils_windows.go there is no such declaration. and it compiles fine without.

sorry missed the file name. but in container utils_windows.go there is no such declaration. and it compiles fine without.

earl-warren commented 2025-10-21 07:15:34 +00:00

Contributor

• Copy link
• Copy Markdown

I'm very confused now. It passed CI but... how can it compile when the function is effectively declared twice if not on windows.

There is something trivial I don't see. 🤔

I'm very confused now. It passed CI but... how can it compile when the function is effectively declared twice if not on windows. There is something trivial I don't see. 🤔

volkmnv commented 2025-10-21 07:17:35 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I'm not used to Go, but my wild guess was that '_windows' in the file name must be interpreted as build tag automaticaly.

I'm not used to Go, but my wild guess was that '_windows' in the file name must be interpreted as build tag automaticaly.

👍 1

volkmnv commented 2025-10-22 10:54:37 +00:00

Author

Contributor

• Copy link
• Copy Markdown

@earl-warren so do you still want me to add the build tag to the file content or is it fine via file name?

@earl-warren so do you still want me to add the build tag to the file content or is it fine via file name?

earl-warren commented 2025-10-22 12:32:14 +00:00

Contributor

• Copy link
• Copy Markdown

I have to say I absolutely do not understand how that works as it is. I would need to research why it compiles before I can give an opinion. If you find the answer this can move on. Otherwise it is a mystery blocking my review I'm afraid. I will eventually take the time to figure it out but not right now.

I have to say I absolutely do not understand how that works as it is. I would need to research why it compiles before I can give an opinion. If you find the answer this can move on. Otherwise it is a mystery blocking my review I'm afraid. I will eventually take the time to figure it out but not right now.

mfenniak commented 2025-10-22 12:41:58 +00:00

Owner

• Copy link
• Copy Markdown

If a file's name, after stripping the extension and a possible _test suffix, matches any of the following patterns:

*_GOOS
*_GOARCH
*_GOOS_GOARCH

(example: source_windows_amd64.go) where GOOS and GOARCH represent any known operating system and architecture values respectively, then the file is considered to have an implicit build constraint requiring those terms (in addition to any explicit constraints in the file).

https://pkg.go.dev/cmd/go#hdr-Build_constraints

> If a file's name, after stripping the extension and a possible _test suffix, matches any of the following patterns: > > *_GOOS > *_GOARCH > *_GOOS_GOARCH > > (example: source_windows_amd64.go) where GOOS and GOARCH represent any known operating system and architecture values respectively, then the file is considered to have an implicit build constraint requiring those terms (in addition to any explicit constraints in the file). https://pkg.go.dev/cmd/go#hdr-Build_constraints >

earl-warren commented 2025-10-22 12:47:59 +00:00

Contributor

• Copy link
• Copy Markdown

thanks!

thanks!

earl-warren commented 2025-10-22 12:49:51 +00:00

Contributor

• Copy link
• Copy Markdown

So... having one file named act/artifactcache/utils_windows.go and the other act/artifactcache/utils_linux.go with no //go:build ... should work fine, right?

So... having one file named `act/artifactcache/utils_windows.go` and the other `act/artifactcache/utils_linux.go` with no ` //go:build ...` should work fine, right?

mfenniak commented 2025-10-22 12:57:23 +00:00

Owner

• Copy link
• Copy Markdown

Usually _unix is the opposite of _windows, I think. Keeps the BSDs happy. Otherwise, yes.

Usually `_unix` is the opposite of `_windows`, I think. Keeps the BSDs happy. Otherwise, yes.

👍 1

earl-warren commented 2025-10-22 17:07:41 +00:00

Contributor

• Copy link
• Copy Markdown

Right... my head is elsewhere, I'm a poor reviewer right now, thanks for fixing my mistakes.

f3/gof3 – util/exec_unix.go

Line 8 in f3/gof3@8962fd5

//go:build !windows

and

f3/gof3 – util/exec_windows.go

Line 8 in f3/gof3@8962fd5

//go:build windows

is what I had in memory. I now understand the build tag and the special name termination are redundant, reason why it worked here, with the right mixture of name termination and build tag.

Right... my head is elsewhere, I'm a poor reviewer right now, thanks for fixing my mistakes. https://code.forgejo.org/f3/gof3/src/commit/8962fd5c4110bff78f0482afcb9e5d5b5e2029e9/util/exec_unix.go#L8 and https://code.forgejo.org/f3/gof3/src/commit/8962fd5c4110bff78f0482afcb9e5d5b5e2029e9/util/exec_windows.go#L8 is what I had in memory. I now understand the build tag and the special name termination are redundant, reason why it worked here, with the right mixture of name termination and build tag.

volkmnv commented 2025-10-23 14:06:29 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I think it is best how it is done (and done in container/utils.go).
You need different file names anyway but if you would use only file names you have to create one for every platform. So having a default file which excludes by comment and other files which include by name makes sense to me.
If it's done with comments to always do it the same way I would't use _windows in the file name. But that's just my opinion.

I think it is best how it is done (and done in container/utils.go). You need different file names anyway but if you would use only file names you have to create one for every platform. So having a default file which excludes by comment and other files which include by name makes sense to me. If it's done with comments to always do it the same way I would't use _windows in the file name. But that's just my opinion.

earl-warren commented 2025-10-23 15:31:52 +00:00

Contributor

• Copy link
• Copy Markdown

The mixture of build tags and file name extension confused me. Please choose one or the other. If I was confused... others will I'm sure.

The mixture of build tags and file name extension confused me. Please choose one or the other. If I was confused... others will I'm sure.

proton-ab commented 2025-10-23 23:27:31 +00:00

First-time contributor

• Copy link
• Copy Markdown

Do you want separate PR to change it in container/utils.go too?

Do you want separate PR to change it in `container/utils.go` too?

volkmnv commented 2025-10-24 06:13:31 +00:00

Author

Contributor

• Copy link
• Copy Markdown

Alright, I added the built tag comment to utils_windows.go

Alright, I added the built tag comment to utils_windows.go

volkmnv marked this conversation as resolved

volkmnv added 1 commit 2025-10-24 06:12:45 +00:00

Add windows build tag to artifactcache/utils_windows.go bd32eb86b4

volkmnv added 1 commit 2025-10-24 06:13:03 +00:00

Merge branch 'main' into fix-windows-compilation ff1dd918d0

volkmnv requested review from earl-warren 2025-10-24 06:13:51 +00:00

earl-warren approved these changes 2025-10-24 06:59:35 +00:00

earl-warren added the

Windows

label 2025-10-24 06:59:51 +00:00

earl-warren commented 2025-10-24 07:00:22 +00:00

Contributor

• Copy link
• Copy Markdown

Thanks everyone for your patience. I learned something valuable 😁

Thanks everyone for your patience. I learned something valuable 😁

earl-warren merged commit ea961a70c3 into main 2025-10-24 07:00:43 +00:00

earl-warren referenced this pull request from a commit 2025-10-24 07:00:44 +00:00

fix: windows compilation (#1101)

earl-warren deleted branch fix-windows-compilation 2025-10-24 07:00:44 +00:00

volkmnv commented 2025-10-24 08:49:23 +00:00

Author

Contributor

• Copy link
• Copy Markdown

I can only agree with that 😃
Thanks for having this merged even though Windows isn't officially supported here.

I can only agree with that 😃 Thanks for having this merged even though Windows isn't officially supported here.

😆 1

earl-warren commented 2025-10-24 10:24:34 +00:00

Contributor

• Copy link
• Copy Markdown

The general idea is that windows specific (or other proprietary OS) can be included as long as they are sandboxed in a clear way.

It would be very useful if you could you weigh in on the request to make room to prepare for such patches in a windows dedicated repository https://github.com/Crown0815/Forgejo-runner-windows-builder/issues/5 🙏

@Crown0815 FYI

The general idea is that windows specific (or other proprietary OS) can be included as long as they are sandboxed in a clear way. It would be very useful if you could you weigh in on the request to make room to prepare for such patches in a windows dedicated repository https://github.com/Crown0815/Forgejo-runner-windows-builder/issues/5 🙏 @Crown0815 FYI

vadiml1024 referenced this pull request from a commit 2025-10-25 14:35:55 +00:00

fix: windows compilation (#1101)

viceice-bot referenced this pull request from actions/setup-forgejo 2025-11-05 00:02:42 +00:00

Update dependency forgejo/runner to v11.3.0 #736

viceice-bot referenced this pull request 2025-11-05 02:31:23 +00:00

Update forgejo-runner to v11.3.0 #1121

mfenniak referenced this pull request from a commit 2025-11-05 02:44:47 +00:00

Update forgejo-runner to v11.3.0 (#1121)

forgejo-mirror referenced this pull request from a commit 2025-11-05 03:01:28 +00:00

Update module code.forgejo.org/forgejo/runner/v11 to v11.3.0 (forgejo) (#9969)

Sign in to join this conversation.

Reviewers

No reviewers

earl-warren

Labels

Clear labels   

FreeBSD

  

Kind/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Chore

It is no fun but it needs to be done, this is a chore.

  

Kind/DependencyUpdate

for pull requests that are created by renovate when updating a dependency

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

  

Windows

Windows ports is an independent project, see https://github.com/Crown0815/Forgejo-runner-windows-builder

  

linux-powerpc64le

powerpc64le

  

linux-riscv64

  

linux-s390x

  

run-end-to-end-tests

Run https://code.forgejo.org/forgejo/end-to-end with a runner compiled from this pull request

  

run-forgejo-tests

Run https://codeberg.org/forgejo/forgejo tests importing the runner from this pull request

  

run-multi-platform-tests

Label a PR for automatic execution of test suite on arm64 architecture.

No labels

FreeBSD

Kind/Breaking

Kind/Bug

Kind/Chore

Kind/DependencyUpdate

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Windows

linux-powerpc64le

linux-riscv64

linux-s390x

run-end-to-end-tests

run-forgejo-tests

run-multi-platform-tests

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

4 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

forgejo/runner!1101

No description provided.