Move mingw tests from appveyor to github actions #2838

binhdvo · 2021-10-31T02:18:55Z

Moved mingw tests to github actions which allows them to run in parallel. The change of environment surfaced a warning error that's been suppressed.

Cyan4973 · 2021-10-31T03:14:30Z

Good ! With Appveyor CI now down to < 10mn, it should no longer be the iteration bottleneck.

Cyan4973 · 2021-10-31T03:23:54Z

.github/workflows/dev-long-tests.yml

+          sh -c "${{matrix.script}}"
+          IF ("${{matrix.artifact}}" -eq "true")
+          {
+            ECHO Creating artifacts


OK, so that's something I overlooked regarding this topic.

On top of Windows tests, Appveyor was also used to create the Windows binary packages at release time.
It's my understanding that you transferred this responsibility to Github Actions.

How does it work ?
In which cases are artifact created, and how to access them ?

For example, I note that, in this PR, there is a script section called "upload artifacts", but it's empty (runtime 0s).

I think a direct translation from the AppVeyor release artifact process doesn't work, since it seems like AppVeyor will just display any artifacts in the \artifacts folder, whereas I think GH Actions works a bit differently.

There's some documentation here: https://github.com/actions/upload-artifact on how to upload the artifact, it seems you just need an extra step that points to the artifact/directory you'd like to upload.

Ah sorry I thought I'd gotten it working, I've reverted those tests back to appveyor for now and will try to get them working on GH in a followup.

The code looks correct to me,
but just to be on the safe side, have you also been able to test that the artifacts are still generated on Appveyor ?

Artifacts on appveyor branch PR look good:

https://ci.appveyor.com/project/YannCollet/zstd-p0yf0/builds/41368517/job/kckbm0scodyuxp4e/artifacts
https://ci.appveyor.com/project/YannCollet/zstd-p0yf0/builds/41368517/job/s1c9o37ux7qkn38s/artifacts

Excellent ! Thanks @binhdvo !

Hm, there are some appveyor failures now, but they don't look related to these changes, investigating.

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]> Signed-off-by: Forenche <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]> Signed-off-by: Forenche <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]> Signed-off-by: Forenche <[email protected]>

Backport the fix from upstream PR #2838 [0]. Found by the Kernel test robot in [1]. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> [cyberknight777: backport to 4.14] Signed-off-by: Cyber Knight <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> Change-Id: I4857d2129f3ec61698fcff12d23b60600ccf54e3 Signed-off-by: Divyanshu-Modi <[email protected]> Signed-off-by: Divyanshu-Modi <[email protected]> (cherry picked from commit f1a7c1d) Signed-off-by: TogoFire <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> Change-Id: Ida34153c3e1ad3236fc74dca19aa3efdcc0745c1

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> Signed-off-by: Divyanshu-Modi <[email protected]> Change-Id: I05b4b77bb8d8a9e9e3c3bd0d5570596b4e82b0ac Signed-off-by: Divyanshu-Modi <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]> Signed-off-by: Marco Zanin <[email protected]>

The variable `litLengthSum` is only used by an `assert()`, so when asserts are disabled the compiler doesn't see any usage and warns. This issue is already fixed upstream by PR #2838 [0]. It was reported by the Kernel test robot in [1]. Another approach would be to change zstd's disabled `assert()` definition to use the argument in a disabled branch, instead of ignoring the argument. I've avoided this approach because there are some small changes necessary to get zstd to build, and I would want to thoroughly re-test for performance, since that is slightly changing the code in every function in zstd. It seems like a trivial change, but some functions are pretty sensitive to small changes. However, I think it is a valid approach that I would like to see upstream take, so I've opened Issue #2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

facebook-github-bot added the CLA Signed label Oct 31, 2021

Cyan4973 reviewed Oct 31, 2021

View reviewed changes

binhdvo force-pushed the appveyormove branch from 2c8b3d8 to 912f7c3 Compare November 1, 2021 13:57

Cyan4973 approved these changes Nov 1, 2021

View reviewed changes

Move mingw tests from appveyor to github actions

c29482f

binhdvo force-pushed the appveyormove branch from 912f7c3 to c29482f Compare November 1, 2021 23:58

binhdvo merged commit b399b47 into facebook:dev Nov 2, 2021

senhuang42 pushed a commit to senhuang42/zstd that referenced this pull request Nov 3, 2021

Move mingw tests from appveyor to github actions (facebook#2838)

ba77106

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move mingw tests from appveyor to github actions #2838

Move mingw tests from appveyor to github actions #2838

binhdvo commented Oct 31, 2021

Cyan4973 commented Oct 31, 2021

Cyan4973 Oct 31, 2021

senhuang42 Nov 1, 2021

binhdvo Nov 1, 2021 •

edited

Loading

Cyan4973 Nov 1, 2021

binhdvo Nov 1, 2021

Cyan4973 Nov 1, 2021

binhdvo Nov 1, 2021

Move mingw tests from appveyor to github actions #2838

Move mingw tests from appveyor to github actions #2838

Conversation

binhdvo commented Oct 31, 2021

Cyan4973 commented Oct 31, 2021

Cyan4973 Oct 31, 2021

Choose a reason for hiding this comment

senhuang42 Nov 1, 2021

Choose a reason for hiding this comment

binhdvo Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Cyan4973 Nov 1, 2021

Choose a reason for hiding this comment

binhdvo Nov 1, 2021

Choose a reason for hiding this comment

Cyan4973 Nov 1, 2021

Choose a reason for hiding this comment

binhdvo Nov 1, 2021

Choose a reason for hiding this comment

binhdvo Nov 1, 2021 •

edited

Loading