tools-golang icon indicating copy to clipboard operation
tools-golang copied to clipboard

Add tests to CI for Windows (and maybe others)

Open swinslow opened this issue 2 years ago • 7 comments

Since Windows handles filepaths differently (and probably some other things I'm not thinking of), we should also configure the GitHub Actions testing to run on a Windows VM -- if that's doable within GitHub Actions. Should also look into whether to do the same for e.g. MacOS and/or others.

swinslow avatar Mar 26 '22 19:03 swinslow

@swinslow , I can try to handle this issue!

CatalinStratu avatar Mar 27 '22 13:03 CatalinStratu

That's great -- thank you @CatalinStratu!

swinslow avatar Mar 27 '22 15:03 swinslow

Thanks, I'll take care of this task after I'm done with task #120

CatalinStratu avatar Mar 27 '22 15:03 CatalinStratu

It will be necessary to refactor some unit tests. To be able to make all the tests run well on both Linux and Windows image

CatalinStratu avatar Mar 31 '22 14:03 CatalinStratu

Thanks @CatalinStratu. If there are some tests that need refactoring, that makes sense and that's fine to include in a proposed PR.

I'd suggest breaking up refactoring changes into small separate PRs wherever possible so that they can be discussed one by one, rather than one very large PR which would be harder to evaluate.

On the specific failures from your screenshot, note that the verification code and number of Files should be the same regardless of whether the Golang tools are being run on Windows or on Linux. The fact that they're different here is a good thing; that means that the tests are identifying a problem with the tools-golang that is leading to different results on the different platforms. We should dig into why the results are different, and figure out how to fix the bug in the tools -- rather than changing the tests to hide the bug :)

For this particular test, it looks like this is where the builder package is creating an SPDX document by analyzing a test folder. This particular test folder, testdata/project1, contains a symbolic link file. I'd be willing to bet that's why you're seeing different results on Windows vs. Linux, since (as far as I know) Windows doesn't have a symbolic link concept.

Can you submit a separate issue for this particular test error, so that we can dig into that specifically? Resolving that bug is going to take a bit of digging to see how symbolic links are handled for other SPDX tools, and it's a bit separate from this issue #117 for enabling the Windows tests to run within the GitHub Actions CI system. Thank you!

swinslow avatar Mar 31 '22 15:03 swinslow

Oh, I forgot to mention -- in case you're wondering what this verification code thingy is, section 7.9 of the SPDX spec has more details.

swinslow avatar Mar 31 '22 15:03 swinslow

@swinslow The solution proposed by me can be found in this PR https://github.com/spdx/tools-golang/pull/129

CatalinStratu avatar Apr 01 '22 08:04 CatalinStratu

Hiya, @swinslow and @lumjjb -- a user submitted a PR with a fix for Windows, so I thought I'd look into getting the tests running on Windows. As expected, there are some path separator (and other) issues, but I'm not entirely sure the "right" thing to do here. I don't use any of this functionality, so I really don't want to make changes that would break things for people, but I see a couple of issues:

  1. if I scan a directory on Unix and the same directory on Windows, I get different packages/files/etc. because of the path separators
  2. path excludes are OS-dependent

To me, I would expect scanning the same set of files on either file system would result in the exact same packages, which tells me that we should be normalizing these to Unix paths. This would also lean into supporting unix paths for exclude patterns on both OSes (and possibly also allowing Windows-style exclude paths). But again, I've not used any of these functions, and would like some guidance what people think is the right thing to do here.

Update: SPDX 2.3 spec references the URI spec as the file name, which indicates to me these all should, in fact be normalized to URI-like paths

kzantow avatar May 17 '24 15:05 kzantow