Windows compat tasks #20
Conversation
When running tests on Windows, core utils like grep may not be present, or may not behave the same way. Therefore, implement inline shell logic in python and invoke it from the task file. Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
|
@anchore/tools what do you all think about the approach here, before I spend a bunch more time going further in this direction. Namely:
If we think this is reasonable, I'll take approaches like these forward until we can turn on unit tests in CI for binny, but there will be a lot more work like this, so I wanted input on the direction before doing that, in case someone has a better idea or an objection to one of these approaches. |
There was a problem hiding this comment.
Nice addition Will!
-
Requiring Python as a dev dependency seems reasonable, since Python is very widely used - Agree easier to go to one common area than manage both Bash and Powershell
-
I think replacing the shell sections with python is a good compromise if we're all in on supporting development on Windows.
if runtime.GOOS == "windows" { t.Skip( <some reason >) }
^ This is a really good way to get it working. I don't expect the flip to be 100% from the start. Basically, we should make the lowest hanging fruit of the dev workflow work on Windows and skip some of the more problematic steps to start. We should have enough warning on the repo CI for other builds if tests were skipped on windows local.
Do we think that having things like got = strings.Replace(got, string(os.PathSeparator), "/")
I'm not so sure about this one and would have to see it in practice. It seems like a good idea for the installer test, but if it gets more complex than that it might be confusing to new users writing tests or a dev practice that's easily over looked.
What do you think the next steps are here? Do you want to carve out time together to pair on the second half of this?
Signed-off-by: Keith Zantow <kzantow@gmail.com>
kzantow
force-pushed
the
windows-compat-tasks
branch
from
980a952 to
8fb320e
Compare
| @@ -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), "/") | |||
There was a problem hiding this comment.
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...
| @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) |
There was a problem hiding this comment.
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?
| @#curl -sSfL https://raw.githubusercontent.com/$(OWNER)/$(PROJECT)/main/install.sh | sh -s -- -b $(TOOL_DIR) | ||
| @-mkdir $(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) |
There was a problem hiding this comment.
We're going to have to solve this for every other repo, just not this one since it has binny go code...
| @@ -122,12 +123,12 @@ 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" | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
For now, simply do not support hosted shell script installer on Windows. Signed-off-by: Will Murphy <will.murphy@anchore.com>
There are text files under tool/testdata/store that are used as binary fixtures, including taking SHA256 of the file during unit tests. Prevent git from applying CRLF autocorrection to these files by removing the "text" attribute from them so that their SHA256 will be stable between Windows and other operating systems. Signed-off-by: Will Murphy <will.murphy@anchore.com>
This is a stepping stone towards actual Windows support. Eventually, all validations should be run, and release artifacts should be built for Windows. Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
There are a few paths that we know are not supported on Windows, so a lower coverage threshold is acceptable. Signed-off-by: Will Murphy <will.murphy@anchore.com>
| runs-on: windows-2022 | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
|
|
There was a problem hiding this comment.
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.
This is a step towards supporting windows (see #17).
The main goal of this branch is to make the Taskfile stop depending on shell core utils to define variables, for example
go list ./... | grep -v {{ .OWNER }}/{{ .PROJECT }}/test | tr '\n' ' 'will fail on Windows becausegrepandtrare not guaranteed to be on PATH.Since Python will be available, this branch experiments with python, either in separate files or inline, to perform this work. Requiring Python as a dev dependency seems reasonable, since Python is very widely used. The other choice is maintaining parallel sets of commands in
shandpowershell, but I think contributors are much more likely to be comfortable in Python than powershell. In other words, requiring contributors to install Python seems less onerous than requiring them to learn powershell. Additionally, I've done some experiments in powershell, and a lot of things that are very quick one-liners in sh, (e.g.<something> | grep -v <something-else>ortest -f <some-file>) are pretty lengthy powershell invocations.