[Bug 54656] New: The winscard tests are not run on the GitLab CI
https://bugs.winehq.org/show_bug.cgi?id=54656 Bug ID: 54656 Summary: The winscard tests are not run on the GitLab CI Product: Wine Version: unspecified Hardware: x86-64 OS: Linux Status: NEW Severity: normal Priority: P2 Component: winscard Assignee: wine-bugs(a)winehq.org Reporter: fgouget(a)codeweavers.com Distribution: --- The winscard tests are not run on the GitLab CI: winscard=dll is missing See the gitlab-debian-* Linux results on: http://winetest.dolphin/data/tests/winscard:winscard.html I'm putting this as a Wine bug rather than a Wine GitLab one because I think the fix is to update the tools/gitlab/image.docker file. Also I'd argue this is a regression introduced by the commit that added the dependency on libpcsclite because the Docker image update should have been made there. commit 8490c43f38e306fef7b5fed3ffcb256efd73af58 Author: Hans Leidekker <hans(a)codeweavers.com> AuthorDate: Thu Feb 16 09:56:12 2023 +0100 winscard: Implement SCardEstablish/ReleaseContext() on top of libpcsclite. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 François Gouget <fgouget(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Regression SHA1| |8490c43f38e306fef7b5fed3ffc | |b256efd73af58 Keywords| |regression, source, | |testcase -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #1 from François Gouget <fgouget(a)codeweavers.com> --- I sent a merge request for this which may or may not works. As far as I can tell it did not trigger an update of the build environment so I expect the winscard tests to still be skipped. But if the build environment were to be updated using the new image.docker file I believe it would end up running the tests. I don't know if there's special magic to be done to cause an MR to update the build environment. https://gitlab.winehq.org/wine/wine/-/merge_requests/2362 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #2 from Hans Leidekker <hans(a)meelstraat.net> --- (In reply to François Gouget from comment #0)
The winscard tests are not run on the GitLab CI:
winscard=dll is missing
See the gitlab-debian-* Linux results on: http://winetest.dolphin/data/tests/winscard:winscard.html
I'm putting this as a Wine bug rather than a Wine GitLab one because I think the fix is to update the tools/gitlab/image.docker file.
Also I'd argue this is a regression introduced by the commit that added the dependency on libpcsclite because the Docker image update should have been made there.
I don't see why this is a regression. There were no tests before that commit. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #3 from François Gouget <fgouget(a)codeweavers.com> --- Before the GitLab CI had all the libraries needed to run all the Wine tests. Now it does not. So it's a regression. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #4 from François Gouget <fgouget(a)codeweavers.com> --- More infrastructure may be needed to actually get meaningful results from the tests: if one just installs the libraries the tests immediately get an SCARD_E_NO_SERVICE error and bail out (which makes sense): can't establish context, make sure pcscd is running -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #5 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to François Gouget from comment #3)
Before the GitLab CI had all the libraries needed to run all the Wine tests. Now it does not. So it's a regression.
By this criteria pretty much everything would be a regression, that's not a useful definition. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #6 from Hans Leidekker <hans(a)meelstraat.net> --- (In reply to François Gouget from comment #4)
More infrastructure may be needed to actually get meaningful results from the tests: if one just installs the libraries the tests immediately get an SCARD_E_NO_SERVICE error and bail out (which makes sense):
can't establish context, make sure pcscd is running
Right, to run all tests we'd need pcscd running and a smart card attached. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #7 from François Gouget <fgouget(a)codeweavers.com> --- (In reply to Alexandre Julliard from comment #5)
(In reply to François Gouget from comment #3)
Before the GitLab CI had all the libraries needed to run all the Wine tests. Now it does not. So it's a regression.
By this criteria pretty much everything would be a regression, that's not a useful definition.
By that definition >99% of the commits are not regressions since they don't introduce new Wine dependencies. And I'd argue that now that updating the GitLab CI falls on the shoulders of Wine developers, a commit that fails to update the GitLab CI to test the new code is a regression. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #8 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to François Gouget from comment #7)
By that definition >99% of the commits are not regressions since they don't introduce new Wine dependencies. And I'd argue that now that updating the GitLab CI falls on the shoulders of Wine developers, a commit that fails to update the GitLab CI to test the new code is a regression.
The definition is not limited to dependencies, but yes, >99% of the commits are not regressions, that's the way it should be. That's what makes things like the regression hall of shame useful. If we apply the keyword to any change of behavior that someone doesn't like, it becomes useless. A regression has to be something that can be pointed to a specific piece of code that worked, and got broken by a change, and where the fix will most likely involve refining the change. For instance, a bug in a newly-added dll would not be considered a regression.
From the user's point of view it may be a new problem, since obviously the bug didn't happen when the dll didn't exist, but from the developer's point of view, there's no previous version of the code that worked, so it hasn't regressed. The goal of Bugzilla is to provide meaningful information to developers, and there's no useful information to gather from the fact that a bug didn't happen when the code didn't exist.
So not running a test that didn't exist previously, to test code that didn't exist previously either, should not be considered a regression in the sense of the Bugzilla regression keyword. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #9 from François Gouget <fgouget(a)codeweavers.com> --- Ok. So merge requests that add a Wine dependency do not have to update tools/gitlab to ensure the GitLab CI has the required libraries for the build, so long as the affected code can build without them. Also there is no requirement to update the GitLab CI to ensure the corresponding tests can be run. Is this policy documented somewhere? (I don't see anything on https://wiki.winehq.org/Submitting_Patches) So who's job is it to update the GitLab CI? The GitLab CI maintainer presumably? Does that mean that bugs about missing dependencies should be filed against the "WineHQ Gitlab" product rather than against Wine? (even though it seems to be the fix would be in Wine) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 François Gouget <fgouget(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|regression | Regression SHA1|8490c43f38e306fef7b5fed3ffc | |b256efd73af58 | -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #10 from Alexandre Julliard <julliard(a)winehq.org> --- (In reply to François Gouget from comment #9)
So who's job is it to update the GitLab CI? The GitLab CI maintainer presumably?
Ultimately, yes, but sending a merge request is a good way to get it on my todo list. The way it's configured, the image is re-generated only once the merge request is merged, because we don't want to have a separate image for every fork. And because we use the same image for everything I have to check it by hand first. So it's actually better to make it a separate MR instead of it being part of the MR that adds the dependency. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #11 from François Gouget <fgouget(a)codeweavers.com> --- I added an "Introducing new dependencies" section to the Submitting Patches page to codify this policy. Review / adjust as appropriate. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54656 --- Comment #12 from Hans Leidekker <hans(a)meelstraat.net> --- The docker image contains libpcsclite-dev now (64-bit only) and winscard tests are run on GitLab CI. Can we close this bug? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=54656 Hans Leidekker <hans@meelstraat.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #13 from Hans Leidekker <hans@meelstraat.net> --- Tests are run on CI now. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=54656 Alexandre Julliard <julliard@winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #14 from Alexandre Julliard <julliard@winehq.org> --- Closing bugs fixed in 11.3. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (2)
-
WineHQ Bugzilla -
WineHQ Bugzilla