Windows compatibility fixes #1152
Labels
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
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1152
Loading…
Reference in a new issue
No description provided.
Delete branch "windows/runner:feature/windows-specific-fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR contains two fixes to the production code:
hashFilefunction in an action misinterpreting paths on Windowsvalidatecommand outputting full paths due to OS-unspecific path trimming logicThe remaining changes adopt tests to properly deal with
or in one case separate dedicated Windows and Linux test setups (709a1d3fdbfc8a6149a57fcf00570e842ef4d040)
I tried to keep the changes as minimal as possible and focussed on getting the tests to run again.
I only added skips for tests that I could not get to run due to the Docker requirements, or the different shutdown behaviors on Linux and Windows.
In the future, I will investigate why the defensive
chdirinlinux_container_environment_extensions_test.go(6608fec705f7e7ed48c7ffed3839f2b4c4c1defb) is necessary and whether I can get all of the docker tests (c492fe121e2f2d2eb6dbc8fd5b1cef543437fa99) to run on GitHub ActionsThe passing of all tests on windows has been confirmed here.
Well, I guess I figured out the reason for
0520ff4e05...The issue is reproducible (on Windows) with just this test:
which will crash the test runner with: "testing.Chdir: chdir .: The system cannot find the path specified."
we should configure wine and run windows tests under it if zrypossible 🤔
still a failing test
The issue is the
os.Open(".")(in the implementation oft.Chdir()) is not behaving well on Windows:This test fails with
panic: testing.Chdir: chdir .: The system cannot find the path specified. [recovered, repanicked].This explains, why the original tests were the way they were.
The lint error in the build and test check is therefore incorrect.
How can I suppress it?I think I got it@viceice wrote in #1152 (comment):
Do you feel the tests run on GitHub are not sufficient? I was given to understand that this repository should remain free of Windows specific testing.
251f29ba2ebd79ed196cbd79ed196c919e5c9f1aIs this failure a problem with the runner or something I can influence?
e3f3a3fe0b8029ba6983@Crown0815 wrote in #1152 (comment):
Wine is free and a windows compabillity layer on linux to run windows programs.
So if you're interested in a ci job here which runs tests with wine and some e2e tests with forgejo (linux) and runner (windows, wine) then we can maybe official support compiling runner for windows on release. 🤔
otherwise LGTM
@ -36,3 +36,3 @@} {if v.workDir != "" {t.Chdir(v.workDir)//nolint:usetestingplease add a comment why
t.Chdirdoesn't work on windows. So noboy tries to change this again 😉Good idea!
@viceice wrote in #1152 (comment):
That would actually be cool. I'll look into this. My only experience with Wine so far was about 20 years ago on a MacBook, so I may be a little outdated 😅
@Crown0815 wrote in #1152 (comment):
Wine should be pretty straight to install and modern on trixie
https://wiki.debian.org/Wine
@ -15,12 +17,13 @@ func init() {}func TestImageExistsLocally(t *testing.T) {skip.If(t, runtime.GOOS != "linux") // Windows and macOS cannot natively run Linux containersWould it make sense to copy the build tag from
docker_images.goand use that to conditional compile the tests in the same manner? That seems a little more consistent between code and test to me, especially since the build tags are more complicated than this check.(As an aside, the concept that we can't do this in Windows seems wrong to me -- this test doesn't require running a Linux container natively, it requires connecting to a Docker daemon to issues commands. But I'll leave that aside because the
docker_images.gois conditional compiled so, might as well follow the same strategy for the tests.)It is wrong. I plan to enable the docker tests on windows, but did not succeed yet. For the time being I wanted to ignore the tests.
Is it ok to leave it like this until I figure out a way to run the tests on windows?
@ -23,6 +25,7 @@ import ()func TestDocker(t *testing.T) {skip.If(t, runtime.GOOS != "linux") // Windows and macOS cannot natively run Linux containersditto, re: conditional compile via tags?
see here
@ -39,2 +40,4 @@t.Fail()}}Typical in tests to use
require.NoError(t, err), rather than logging and failing; would updating it to that convention be OK?@ -45,3 +49,4 @@}} else {cwd, err := os.Getwd()if err != nil {Seems odd that this one doesn't fail the test on an error; that would make it easier to identify any later test failures caused by the cwd being unexpected. But not a big deal.
@ -21,3 +23,4 @@//go:generate mockery --srcpkg=github.com/sirupsen/logrus --name=FieldLoggerfunc TestJobExecutor(t *testing.T) {skip.If(t, runtime.GOOS != "linux") // Windows and macOS cannot natively run Linux containersWhat would you think of abstracting this check into a utility method, like
skip.If(t, util.NoDocker()), and then implement theNoDockercheck with the same logic as the build tags? Then it doesn't require a comment because it's clear why it's being skipped, as the logic changes for different platforms it doesn't need to be done in different locations (eg. #1135), and if we do want to use a docker client against a remote daemon it would be possible to set up the environment and updateNoDockerto check that environment (not necessary now).I agree, but believe it would be better to "just" make the tests work on Windows as well by making the docker daemon available.
(see here).
I'll see that this is the next thing I tackle.
In the end, using Docker on Windows will be very useful.
2087e0eb3df2769d90e6@mfenniak, I implemented the changes (except for the non-Linux skips). Let me know if there is anything else.
Windows-specific-fixesto Windows compatibility fixes