Re: [Piglit] [PATCH] framework/test/deqp: handle deqp extra args properly in more cases
Reviewed-by: Mark Janes
Dylan Baker writes:
> Currently we blindly pass deqp extra args to each test case, and only to
> the test case.
>
> There are two cases where this is problematic:
> 1) if the extra args need to be passed to the deqp command that
>generates the test list (it isn't before this patch)
> 2) if there are any --deqp-case options passed into the Test derived
>classes this will cause the tests to fail (conflicting options)
>
> Both of these are resolved by this patch.
>
> CC: Mark Janes
> Signed-off-by: Dylan Baker
> ---
> framework/test/deqp.py | 11 ---
> tests/deqp_gles2.py| 12 +++-
> tests/deqp_gles3.py| 11 +++
> tests/deqp_gles31.py | 12 +++-
> 4 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/framework/test/deqp.py b/framework/test/deqp.py
> index 843cde8..1462ca2 100644
> --- a/framework/test/deqp.py
> +++ b/framework/test/deqp.py
> @@ -61,8 +61,12 @@ def get_option(env_varname, config_option, default=None):
> return opt or default
>
>
> -def gen_caselist_txt(bin_, caselist):
> -"""Generate a caselist.txt and return its path."""
> +def gen_caselist_txt(bin_, caselist, extra_args):
> +"""Generate a caselist.txt and return its path.
> +
> +Extra args should be a list of extra arguments to pass to deqp.
> +
> +"""
> # dEQP is stupid (2014-12-07):
> # 1. To generate the caselist file, dEQP requires that the process's
> # current directory must be that same as that of the executable.
> @@ -76,9 +80,10 @@ def gen_caselist_txt(bin_, caselist):
> # we must *dynamically* generate it during the testrun.
> basedir = os.path.dirname(bin_)
> caselist_path = os.path.join(basedir, caselist)
> +
> # TODO: need to catch some exceptions here...
> subprocess.check_call(
> -[bin_, '--deqp-runmode=txt-caselist'], cwd=basedir)
> +[bin_, '--deqp-runmode=txt-caselist'] + extra_args, cwd=basedir)
> assert os.path.exists(caselist_path)
> return caselist_path
>
> diff --git a/tests/deqp_gles2.py b/tests/deqp_gles2.py
> index b97cb5d..c4bacb2 100644
> --- a/tests/deqp_gles2.py
> +++ b/tests/deqp_gles2.py
> @@ -24,20 +24,22 @@ from framework.test import deqp
>
> __all__ = ['profile']
>
> -
> # Path to the deqp-gles2 executable.
> _DEQP_GLES2_BIN = deqp.get_option('PIGLIT_DEQP_GLES2_BIN',
>('deqp-gles2', 'bin'))
>
> +_EXTRA_ARGS = deqp.get_option('PIGLIT_DEQP_GLES2_EXTRA_ARGS',
> + ('deqp-gles2', 'extra_args'),
> + default='').split()
> +
>
> class DEQPGLES2Test(deqp.DEQPBaseTest):
> deqp_bin = _DEQP_GLES2_BIN
> -extra_args = deqp.get_option('PIGLIT_DEQP_GLES2_EXTRA_ARGS',
> - ('deqp-gles2', 'extra_args'),
> - default='').split()
> +extra_args = [x for x in _EXTRA_ARGS if not x.startswith('--deqp-case')]
>
>
> profile = deqp.make_profile( # pylint: disable=invalid-name
> deqp.iter_deqp_test_cases(
> -deqp.gen_caselist_txt(_DEQP_GLES2_BIN, 'dEQP-GLES2-cases.txt')),
> +deqp.gen_caselist_txt(_DEQP_GLES2_BIN, 'dEQP-GLES2-cases.txt',
> + _EXTRA_ARGS)),
> DEQPGLES2Test)
> diff --git a/tests/deqp_gles3.py b/tests/deqp_gles3.py
> index dfb82c9..d353899 100644
> --- a/tests/deqp_gles3.py
> +++ b/tests/deqp_gles3.py
> @@ -35,6 +35,10 @@ _DEQP_GLES3_EXE = deqp.get_option('PIGLIT_DEQP_GLES3_EXE',
> _DEQP_MUSTPASS = deqp.get_option('PIGLIT_DEQP_MUSTPASS',
> ('deqp-gles3', 'mustpasslist'))
>
> +_EXTRA_ARGS = deqp.get_option('PIGLIT_DEQP_GLES3_EXTRA_ARGS',
> + ('deqp-gles3', 'extra_args'),
> + default='').split()
> +
>
> def _get_test_case(root, root_group, outputfile):
> """Parser the test case list of Google Android CTS,
> @@ -73,9 +77,7 @@ def filter_mustpass(caselist_path):
>
> class DEQPGLES3Test(deqp.DEQPBaseTest):
> deqp_bin = _DEQP_GLES3_EXE
> -extra_args = deqp.get_option('PIGLIT_DEQP_GLES3_EXTRA_ARGS',
> - ('deqp-gles3', 'extra_args'),
> - default='').split()
> +extra_args = [x for x in _EXTRA_ARGS if not x.startswith('--deqp-case')]
>
>
> def __init__(self, *args, **kwargs):
> @@ -84,5 +86,6 @@ class DEQPGLES3Test(deqp.DEQPBaseTest):
>
> profile = deqp.make_profile( # pylint: disable=invalid-name
> deqp.iter_deqp_test_cases(filter_mustpass(
> -deqp.gen_caselist_txt(_DEQP_GLES3_EXE, 'dEQP-GLES3-cases.txt'))),
> +deqp.gen_caselist_txt(_DEQP_GLES3_EXE, 'dEQP-GLES3-cases.txt',
> + _EXTRA_ARGS))),
> DEQPGLES3Test)
> diff --git a/tests/deqp_gles31.py b/tests/deqp_gles31.py
> index 423c1bf..b98d2be 100644
> --- a/tests/deqp_gles31.py
> +++ b/tests/deqp_gles31
[Piglit] [PATCH] framework/test/deqp: handle deqp extra args properly in more cases
Currently we blindly pass deqp extra args to each test case, and only to
the test case.
There are two cases where this is problematic:
1) if the extra args need to be passed to the deqp command that
generates the test list (it isn't before this patch)
2) if there are any --deqp-case options passed into the Test derived
classes this will cause the tests to fail (conflicting options)
Both of these are resolved by this patch.
CC: Mark Janes
Signed-off-by: Dylan Baker
---
framework/test/deqp.py | 11 ---
tests/deqp_gles2.py| 12 +++-
tests/deqp_gles3.py| 11 +++
tests/deqp_gles31.py | 12 +++-
4 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/framework/test/deqp.py b/framework/test/deqp.py
index 843cde8..1462ca2 100644
--- a/framework/test/deqp.py
+++ b/framework/test/deqp.py
@@ -61,8 +61,12 @@ def get_option(env_varname, config_option, default=None):
return opt or default
-def gen_caselist_txt(bin_, caselist):
-"""Generate a caselist.txt and return its path."""
+def gen_caselist_txt(bin_, caselist, extra_args):
+"""Generate a caselist.txt and return its path.
+
+Extra args should be a list of extra arguments to pass to deqp.
+
+"""
# dEQP is stupid (2014-12-07):
# 1. To generate the caselist file, dEQP requires that the process's
# current directory must be that same as that of the executable.
@@ -76,9 +80,10 @@ def gen_caselist_txt(bin_, caselist):
# we must *dynamically* generate it during the testrun.
basedir = os.path.dirname(bin_)
caselist_path = os.path.join(basedir, caselist)
+
# TODO: need to catch some exceptions here...
subprocess.check_call(
-[bin_, '--deqp-runmode=txt-caselist'], cwd=basedir)
+[bin_, '--deqp-runmode=txt-caselist'] + extra_args, cwd=basedir)
assert os.path.exists(caselist_path)
return caselist_path
diff --git a/tests/deqp_gles2.py b/tests/deqp_gles2.py
index b97cb5d..c4bacb2 100644
--- a/tests/deqp_gles2.py
+++ b/tests/deqp_gles2.py
@@ -24,20 +24,22 @@ from framework.test import deqp
__all__ = ['profile']
-
# Path to the deqp-gles2 executable.
_DEQP_GLES2_BIN = deqp.get_option('PIGLIT_DEQP_GLES2_BIN',
('deqp-gles2', 'bin'))
+_EXTRA_ARGS = deqp.get_option('PIGLIT_DEQP_GLES2_EXTRA_ARGS',
+ ('deqp-gles2', 'extra_args'),
+ default='').split()
+
class DEQPGLES2Test(deqp.DEQPBaseTest):
deqp_bin = _DEQP_GLES2_BIN
-extra_args = deqp.get_option('PIGLIT_DEQP_GLES2_EXTRA_ARGS',
- ('deqp-gles2', 'extra_args'),
- default='').split()
+extra_args = [x for x in _EXTRA_ARGS if not x.startswith('--deqp-case')]
profile = deqp.make_profile( # pylint: disable=invalid-name
deqp.iter_deqp_test_cases(
-deqp.gen_caselist_txt(_DEQP_GLES2_BIN, 'dEQP-GLES2-cases.txt')),
+deqp.gen_caselist_txt(_DEQP_GLES2_BIN, 'dEQP-GLES2-cases.txt',
+ _EXTRA_ARGS)),
DEQPGLES2Test)
diff --git a/tests/deqp_gles3.py b/tests/deqp_gles3.py
index dfb82c9..d353899 100644
--- a/tests/deqp_gles3.py
+++ b/tests/deqp_gles3.py
@@ -35,6 +35,10 @@ _DEQP_GLES3_EXE = deqp.get_option('PIGLIT_DEQP_GLES3_EXE',
_DEQP_MUSTPASS = deqp.get_option('PIGLIT_DEQP_MUSTPASS',
('deqp-gles3', 'mustpasslist'))
+_EXTRA_ARGS = deqp.get_option('PIGLIT_DEQP_GLES3_EXTRA_ARGS',
+ ('deqp-gles3', 'extra_args'),
+ default='').split()
+
def _get_test_case(root, root_group, outputfile):
"""Parser the test case list of Google Android CTS,
@@ -73,9 +77,7 @@ def filter_mustpass(caselist_path):
class DEQPGLES3Test(deqp.DEQPBaseTest):
deqp_bin = _DEQP_GLES3_EXE
-extra_args = deqp.get_option('PIGLIT_DEQP_GLES3_EXTRA_ARGS',
- ('deqp-gles3', 'extra_args'),
- default='').split()
+extra_args = [x for x in _EXTRA_ARGS if not x.startswith('--deqp-case')]
def __init__(self, *args, **kwargs):
@@ -84,5 +86,6 @@ class DEQPGLES3Test(deqp.DEQPBaseTest):
profile = deqp.make_profile( # pylint: disable=invalid-name
deqp.iter_deqp_test_cases(filter_mustpass(
-deqp.gen_caselist_txt(_DEQP_GLES3_EXE, 'dEQP-GLES3-cases.txt'))),
+deqp.gen_caselist_txt(_DEQP_GLES3_EXE, 'dEQP-GLES3-cases.txt',
+ _EXTRA_ARGS))),
DEQPGLES3Test)
diff --git a/tests/deqp_gles31.py b/tests/deqp_gles31.py
index 423c1bf..b98d2be 100644
--- a/tests/deqp_gles31.py
+++ b/tests/deqp_gles31.py
@@ -24,20 +24,22 @@ from framework.test import deqp
__all__ = ['profile']
-
# Path to the deqp-gles3 executable.
_DEQP_GLES31_BIN = deqp.get_option('PIGLIT_DEQP_GLES31_BIN',
('deqp-gles31', 'bin'))
+_EXTRA_ARGS = deqp.get_option('PIGLIT_
[Piglit] [PATCH] framework: Catch trying to load a non-existant profile
Currently if one specifies a non-existant module (ie: piglit run
deqp_gles4 out), then an uncaught ImportError will be raised. This patch
fixes that by catching the exception and re-raising a PiglitFatalError.
Signed-off-by: Dylan Baker
---
framework/profile.py | 11 +--
framework/tests/profile_tests.py | 6 ++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/framework/profile.py b/framework/profile.py
index 66d7899..bce28b9 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -411,7 +411,7 @@ class TestProfile(object):
def load_test_profile(filename):
-""" Load a python module and return it's profile attribute
+"""Load a python module and return it's profile attribute.
All of the python test files provide a profile attribute which is a
TestProfile instance. This loads that module and returns it or raises an
@@ -419,7 +419,10 @@ def load_test_profile(filename):
This method doesn't care about file extensions as a way to be backwards
compatible with script wrapping piglit. 'tests/quick', 'tests/quick.tests',
-and 'tests/quick.py' are all equally valid for filename
+and 'tests/quick.py' are all equally valid for filename.
+
+This will raise a FatalError if the module doesn't exist, or if the module
+doesn't have a profile attribute.
Arguments:
filename -- the name of a python module to get a 'profile' from
@@ -433,6 +436,10 @@ def load_test_profile(filename):
raise exceptions.PiglitFatalError(
'There is not profile attribute in module {}.\n'
'Did you specify the right file?'.format(filename))
+except ImportError:
+raise exceptions.PiglitFatalError(
+'There is no test profile called "{}".\n'
+'Check your spelling?'.format(filename))
def merge_test_profiles(profiles):
diff --git a/framework/tests/profile_tests.py b/framework/tests/profile_tests.py
index 2488e7b..765528e 100644
--- a/framework/tests/profile_tests.py
+++ b/framework/tests/profile_tests.py
@@ -52,6 +52,12 @@ def test_load_test_profile_no_profile():
profile.load_test_profile('__init__')
+@nt.raises(exceptions.PiglitFatalError)
+def test_load_test_profile_no_module():
+"""profile.load_test_profile: Trying to load a non-existant module exits"""
+profile.load_test_profile('this_module_will_never_ever_exist')
+
+
def test_load_test_profile_returns():
"""profile.load_test_profile: returns a TestProfile instance"""
profile_ = profile.load_test_profile('sanity')
--
2.4.3
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] framework/test/base: Recognize returncode 3 as crash on Windows.
Reviewed-by: Dylan Baker On Fri, Jun 19, 2015 at 08:33:11PM +0100, Jose Fonseca wrote: > Programs that terminate via MSVCRT's abort(), including failed > assertions, return code 3, not a negative number. > > This is particularly pertinent now that the use of the standard assert > macro has increased in (over gallium's assert macro, which would trap > the debugger with INT3, and return a negative exception number.) > --- > framework/test/base.py | 15 ++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/framework/test/base.py b/framework/test/base.py > index f29fc94..2dc0f4b 100644 > --- a/framework/test/base.py > +++ b/framework/test/base.py > @@ -109,6 +109,19 @@ class ProcessTimeout(threading.Thread): > return self.status > > > +def _is_crash_returncode(returncode): > +"""Determine whether the given process return code correspond to a > +crash.""" > +if sys.platform == 'win32': > +# On Windows: > +# - For uncaught exceptions the process terminates with the exception > +# code, which is usually negative > +# - MSVCRT's abort() terminates process with exit code 3 > +return returncode < 0 or returncode == 3 > +else: > +return returncode < 0 > + > + > class Test(object): > """ Abstract base class for Test classes > > @@ -228,7 +241,7 @@ class Test(object): > > self.interpret_result() > > -if self.result['returncode'] < 0: > +if _is_crash_returncode(self.result['returncode']): > # check if the process was terminated by the timeout > if self.timeout > 0 and self.__proc_timeout.join() > 0: > self.result['result'] = 'timeout' > -- > 2.1.0 > signature.asc Description: Digital signature ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] arb_gpu_shader_fp64: verify function parameter overloads
On Mon, Jun 22, 2015 at 12:34 PM, Ian Romanick wrote:
> On 06/22/2015 09:25 AM, Ilia Mirkin wrote:
>> On Mon, Jun 22, 2015 at 12:22 PM, Ian Romanick wrote:
>>> On 06/17/2015 12:43 PM, Ilia Mirkin wrote:
fp64 adds variants of all sorts of functions. make sure that we're still
able to resolve otherwise-ambiguous calls in the presence of gs5.
Signed-off-by: Ilia Mirkin
---
.../compiler/implicit-conversions-func.vert| 22
++
1 file changed, 22 insertions(+)
create mode 100644
tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
diff --git
a/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
new file mode 100644
index 000..224aecb
--- /dev/null
+++
b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
@@ -0,0 +1,22 @@
+// [config]
+// expect_result: pass
+// glsl_version: 1.50
+// require_extensions: GL_ARB_gpu_shader_fp64 GL_ARB_gpu_shader5
+// [end config]
+
+// Test that overloaded function selection still works in the presence
+// of new double variants. ARB_gpu_shader5 provides the overload
+// selection rules that allow the implementatino to pick one of the
>>> implementation
>>>
+// candidates.
+
+#version 150
+#extension GL_ARB_gpu_shader_fp64: enable
+#extension GL_ARB_gpu_shader5: enable
+
+void foo(double a);
+void foo(float a);
+
+void test() {
+ mod(5, 6);
+ foo(5);
+}
>>>
>>> Which overload does the spec say is supposed to be used for foo(5)? We
>>> should provide an implementation for that overload, and make this a
>>> linker test.
>>
>> The float one, I think. From ARB_gpu_shader5, see point #3:
>
> That was going to be my guess... mostly since that means the selected
> overload wouldn't "change" with the addition of ARB_gpu_shader_fp64.
> You can imagine a shader like:
>
> #version 150
> #extension GL_ARB_gpu_shader_fp64: enable
> #extension GL_ARB_gpu_shader5: enable
>
> #ifdef GL_ARB_gpu_shader_fp64
> void foo(double a)
> {
> ...
> }
> #endif
>
> void foo(float a)
> {
> ...
> }
>
> void main()
> {
>foo(5);
> }
>
> It should be the same whether ARB_gpu_shader_fp64 is supported or not.
Yeah, although the fun thing is that without the gs5 enable, it goes
from "working" (because GLSL says that you can always
implicitly convert an int to a float) without doubles to "compile
failure" with doubles because it can't resolve which overload to pick
between the float and double one. I'm not 100% sure if that's the
spec'd behaviour, but it's definitely mesa's.
-ilia
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] arb_gpu_shader_fp64: verify function parameter overloads
On 06/22/2015 09:25 AM, Ilia Mirkin wrote:
> On Mon, Jun 22, 2015 at 12:22 PM, Ian Romanick wrote:
>> On 06/17/2015 12:43 PM, Ilia Mirkin wrote:
>>> fp64 adds variants of all sorts of functions. make sure that we're still
>>> able to resolve otherwise-ambiguous calls in the presence of gs5.
>>>
>>> Signed-off-by: Ilia Mirkin
>>> ---
>>> .../compiler/implicit-conversions-func.vert| 22
>>> ++
>>> 1 file changed, 22 insertions(+)
>>> create mode 100644
>>> tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>>>
>>> diff --git
>>> a/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>>> b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>>> new file mode 100644
>>> index 000..224aecb
>>> --- /dev/null
>>> +++ b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>>> @@ -0,0 +1,22 @@
>>> +// [config]
>>> +// expect_result: pass
>>> +// glsl_version: 1.50
>>> +// require_extensions: GL_ARB_gpu_shader_fp64 GL_ARB_gpu_shader5
>>> +// [end config]
>>> +
>>> +// Test that overloaded function selection still works in the presence
>>> +// of new double variants. ARB_gpu_shader5 provides the overload
>>> +// selection rules that allow the implementatino to pick one of the
>> implementation
>>
>>> +// candidates.
>>> +
>>> +#version 150
>>> +#extension GL_ARB_gpu_shader_fp64: enable
>>> +#extension GL_ARB_gpu_shader5: enable
>>> +
>>> +void foo(double a);
>>> +void foo(float a);
>>> +
>>> +void test() {
>>> + mod(5, 6);
>>> + foo(5);
>>> +}
>>>
>>
>> Which overload does the spec say is supposed to be used for foo(5)? We
>> should provide an implementation for that overload, and make this a
>> linker test.
>
> The float one, I think. From ARB_gpu_shader5, see point #3:
That was going to be my guess... mostly since that means the selected
overload wouldn't "change" with the addition of ARB_gpu_shader_fp64.
You can imagine a shader like:
#version 150
#extension GL_ARB_gpu_shader_fp64: enable
#extension GL_ARB_gpu_shader5: enable
#ifdef GL_ARB_gpu_shader_fp64
void foo(double a)
{
...
}
#endif
void foo(float a)
{
...
}
void main()
{
foo(5);
}
It should be the same whether ARB_gpu_shader_fp64 is supported or not.
> To determine whether the conversion for a single argument in one match is
> better than that for another match, the following rules are applied, in
> order:
>
>1. An exact match is better than a match involving any implicit
> conversion.
>
>2. A match involving an implicit conversion from float to double is
> better than a match involving any other implicit conversion.
>
>3. A match involving an implicit conversion from either int or uint to
> float is better than a match involving an implicit conversion from
> either int or uint to double.
>
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] Weekly 10 Picks from Patchwork for review and friendly reminder to clean out your old patches
On 06/19/2015 01:12 PM, Jose Fonseca wrote: > On 19/06/15 20:45, Ilia Mirkin wrote: >> On Fri, Jun 19, 2015 at 3:39 PM, Jose Fonseca >> wrote: >>> On 19/06/15 13:32, Timothy Arceri wrote: Hi all, Unfortunately since its introduction patchwork hasn't seen a lot of love in the Piglit and Mesa projects so I thought I'd try something out to bring it out of the shadows and into the limelight. The idea is simple we have many useful but long forgotten patches sitting on the mailing list that would serve us much better sitting in the git repo, so once a week I (or anyone else that wants to help out) would pick 10 seemingly random older patches that could do with a review/update/etc. I'm hoping this will help with both clearing out the backlog of patches and getting people thinking about patchwork. I'm interested in feedback on what people think about this idea. >>> >>> >>> Patchwork seems to fail to recognize submited patches. Eg. one of my >>> patches is https://patchwork.freedesktop.org/patch/51379/ but it has >>> been >>> commited on >>> http://cgit.freedesktop.org/piglit/commit/?id=540972b46e51ee1d4acbb3757b731a066e2b6ba5 >>> >>> >>> Why is that? >> >> It's very strict about matching patches. The diff has to be identical. >> Which it often isn't if you made minor changes, or rebased, or >> whatever. > > Without a bit of fuzzy matching I'm afraid this looks a bit hopeless to me: FYI... git-push will tell you which patches were recognized and which ones were not. That generally makes it easy to know you need to go in and manually mark some. > I believe the bulk of the patches are committed, and only a few is > forgotten. Looking at the patchwork backlog it's fair to say a large > portion of those committed don't get detected due to small changes. So > the end result is that developers have to click through and babysit the > bulk of their changes in patchwork, so that the few truly forgotten > patches get to stand out? > > I don't think this will ever going to work. There's no incentive in the > system for the most prolific developers to spend so much of their time, > for the sake of the occasional contributor. The patchwork system seems > bound to echo what happens on the mailing list: their patches get to be > lost twice... > > > There 's another concern -- one can only change the status of our own > patches. So if one commits on behalf of somebody else, and that patch > doesn't get recognized, one needs to get that other person to register > and click through patchwork? > > > > > > I wonder if it wouldn't be better to have a more comprehensive solution > for review and tracking, ala github pull requests. Maybe have an > official mirror for mesa/piglit in github, or deploy gitlab > (https://about.gitlab.com/features/) in fdo.org, or something along > those lines, and start tracking this sort of things as pull requests. > > I known it might look (and be) a wild idea at the moment, but I believe > this will be the future eventually: with things like cloud-based CI > systems (Travis CI, AppVeyor), projects can have testsuites run > automatically on pull requests (No GPU HW available, but one can still > ensure builds don't fail, run unit tests, and even rendering tests with > SW renderers) and detect issues even before reviewing or committing. > > I've seen this happen first-handed: I once make a pull request to an > open-source project I had never contributed on github, a few minutes > later bot added a comment saying that the project built fine and all > unit tests passed, and all the maintainer had to do was clicking a button. > > I'm now trying to repro this on some of my open source projects. (E.g, > Apitrace). I still have a long way to go, but already it is showing > fruits -- I immediately know when a Linux developr proposes a Apitrace > change that breaks Windows vuild (or a Windows developer breaks Linux > build) , and I can point them to the logs and they can often fix them > selves. I hope one day I have unit tests and more there too. > > > Jose > ___ > Piglit mailing list > Piglit@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/piglit > ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] arb_gpu_shader_fp64: verify function parameter overloads
On Mon, Jun 22, 2015 at 12:22 PM, Ian Romanick wrote:
> On 06/17/2015 12:43 PM, Ilia Mirkin wrote:
>> fp64 adds variants of all sorts of functions. make sure that we're still
>> able to resolve otherwise-ambiguous calls in the presence of gs5.
>>
>> Signed-off-by: Ilia Mirkin
>> ---
>> .../compiler/implicit-conversions-func.vert| 22
>> ++
>> 1 file changed, 22 insertions(+)
>> create mode 100644
>> tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>>
>> diff --git
>> a/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>> b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>> new file mode 100644
>> index 000..224aecb
>> --- /dev/null
>> +++ b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>> @@ -0,0 +1,22 @@
>> +// [config]
>> +// expect_result: pass
>> +// glsl_version: 1.50
>> +// require_extensions: GL_ARB_gpu_shader_fp64 GL_ARB_gpu_shader5
>> +// [end config]
>> +
>> +// Test that overloaded function selection still works in the presence
>> +// of new double variants. ARB_gpu_shader5 provides the overload
>> +// selection rules that allow the implementatino to pick one of the
> implementation
>
>> +// candidates.
>> +
>> +#version 150
>> +#extension GL_ARB_gpu_shader_fp64: enable
>> +#extension GL_ARB_gpu_shader5: enable
>> +
>> +void foo(double a);
>> +void foo(float a);
>> +
>> +void test() {
>> + mod(5, 6);
>> + foo(5);
>> +}
>>
>
> Which overload does the spec say is supposed to be used for foo(5)? We
> should provide an implementation for that overload, and make this a
> linker test.
The float one, I think. From ARB_gpu_shader5, see point #3:
To determine whether the conversion for a single argument in one match is
better than that for another match, the following rules are applied, in
order:
1. An exact match is better than a match involving any implicit
conversion.
2. A match involving an implicit conversion from float to double is
better than a match involving any other implicit conversion.
3. A match involving an implicit conversion from either int or uint to
float is better than a match involving an implicit conversion from
either int or uint to double.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] arb_gpu_shader_fp64: verify function parameter overloads
On 06/17/2015 12:43 PM, Ilia Mirkin wrote:
> fp64 adds variants of all sorts of functions. make sure that we're still
> able to resolve otherwise-ambiguous calls in the presence of gs5.
>
> Signed-off-by: Ilia Mirkin
> ---
> .../compiler/implicit-conversions-func.vert| 22
> ++
> 1 file changed, 22 insertions(+)
> create mode 100644
> tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
>
> diff --git
> a/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
> b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
> new file mode 100644
> index 000..224aecb
> --- /dev/null
> +++ b/tests/spec/arb_gpu_shader_fp64/compiler/implicit-conversions-func.vert
> @@ -0,0 +1,22 @@
> +// [config]
> +// expect_result: pass
> +// glsl_version: 1.50
> +// require_extensions: GL_ARB_gpu_shader_fp64 GL_ARB_gpu_shader5
> +// [end config]
> +
> +// Test that overloaded function selection still works in the presence
> +// of new double variants. ARB_gpu_shader5 provides the overload
> +// selection rules that allow the implementatino to pick one of the
implementation
> +// candidates.
> +
> +#version 150
> +#extension GL_ARB_gpu_shader_fp64: enable
> +#extension GL_ARB_gpu_shader5: enable
> +
> +void foo(double a);
> +void foo(float a);
> +
> +void test() {
> + mod(5, 6);
> + foo(5);
> +}
>
Which overload does the spec say is supposed to be used for foo(5)? We
should provide an implementation for that overload, and make this a
linker test.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH] gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides
The draw-vertices tests exercises unusual vertex sizes and strides,
but not with interleaved arrays.
This test creates a VBO with interleaved vertex positions and colors.
The colors are positioned at offsets like 9, 10 and 11 bytes from the
start of the vertex. Unusual strides between vertices are tested too.
Exercises a bug in Gallium's u_vbuf code where the driver was passed
pipe_vertex_element::src_offset values which weren't a multiple of
four, even when PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY
returned 1.
v2: add more comments, specifically describing Flockers. Return "warn"
if a test fails, not "fail".
Reviewed-by: Marek Olšák
---
tests/all.py | 1 +
tests/spec/gl-1.5/CMakeLists.gl.txt | 1 +
tests/spec/gl-1.5/vertex-buffer-offsets.c | 149 ++
3 files changed, 151 insertions(+)
create mode 100644 tests/spec/gl-1.5/vertex-buffer-offsets.c
diff --git a/tests/all.py b/tests/all.py
index 3e91055..6c4fb41 100644
--- a/tests/all.py
+++ b/tests/all.py
@@ -1049,6 +1049,7 @@ with profile.group_manager(
'normal3b3s-invariance-byte', run_concurrent=False)
g(['gl-1.5-normal3b3s-invariance', 'GL_SHORT'],
'normal3b3s-invariance-short', run_concurrent=False)
+g(['gl-1.5-vertex-buffer-offsets'], 'vertex-buffer-offsets')
with profile.group_manager(
PiglitGLTest,
diff --git a/tests/spec/gl-1.5/CMakeLists.gl.txt
b/tests/spec/gl-1.5/CMakeLists.gl.txt
index 8dcd95d..f10c6cb 100644
--- a/tests/spec/gl-1.5/CMakeLists.gl.txt
+++ b/tests/spec/gl-1.5/CMakeLists.gl.txt
@@ -10,5 +10,6 @@ link_libraries (
)
piglit_add_executable (gl-1.5-normal3b3s-invariance normal3b3s-invariance.c)
+piglit_add_executable (gl-1.5-vertex-buffer-offsets vertex-buffer-offsets.c)
# vim: ft=cmake:
diff --git a/tests/spec/gl-1.5/vertex-buffer-offsets.c
b/tests/spec/gl-1.5/vertex-buffer-offsets.c
new file mode 100644
index 000..1b9580d
--- /dev/null
+++ b/tests/spec/gl-1.5/vertex-buffer-offsets.c
@@ -0,0 +1,149 @@
+/*
+ * Copyright 2015 VMware, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/*
+ * Test interleaved vertex arrays with unusual element offsets and strides.
+ *
+ * The game Flockers (from Steam) uses some unusual vertex arrays.
+ * For example: glVertexAttribPointerARB(index = 1, size = 3, type = GL_FLOAT,
+ * normalized = GL_FALSE, stride = 87, pointer = 0x4b). Note that the
+ * offset to the float[3] attribute is 75 (0x4b) bytes and the stride between
+ * vertices is 87 bytes.
+ *
+ * According to the OpenGL specification, OpenGL 1.5, page 33:
+ * "Clients must align data elements consistent with the requirements of the
+ * client platform, with an additional base-level requirement that an offset
+ * within a buffer to a datum comprising N basic machine units be a multiple
+ * of N."
+ *
+ * However, the spec does not say what might happen if that requirement is
+ * not met. There is no language about raising a GL error or undefined
+ * behavior.
+ *
+ * This test exercises float[3] attributes at unusual offsets/strides.
+ * If a failure is detected we generate "warn" instead of "fail" since
+ * according to the spec, the failure is allowed, but there are apps (such
+ * as Flockers) that will hit this issue.
+ *
+ * If a failure/warning is reported, the OpenGL implementor will have to
+ * decide if conformance or app support is more important.
+ *
+ * Brian Paul
+ */
+
+#include "piglit-util-gl.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+ config.supports_gl_compat_version = 15;
+ config.window_visual = PIGLIT_GL_VISUAL_RGBA | PIGLIT_GL_VISUAL_DOUBLE;
+PIGLIT_GL_TEST_CONFIG_END
+
+
+void
+piglit_init(int argc, char **argv)
+{
+ /* nothing */
+}
+
+
+static bool
+test_offset_stride(int color_offset, int stride)
+{
+ static const GLfloat vertex[4][2] = {
+ { -1, -1 },
+ { 1, -1 },
+
Re: [Piglit] [PATCH] gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides
OK, my piglit test does test what Flockers is doing. At the failure point there are 8 vertex arrays in use. Using _mesa_print_arrays(): Vertex: Ptr=, Type=GL_FLOAT, Size=3, ElemSize=12, Stride=87, Buffer=1041(Size 21489) Normal: Ptr=000C, Type=GL_FLOAT, Size=3, ElemSize=12, Stride=87, Buffer=1041(Size 21489) Color: Ptr=0030, Type=GL_FLOAT, Size=4, ElemSize=16, Stride=87, Buffer=1041(Size 21489) TexCoord[0]: Ptr=0040, Type=GL_FLOAT, Size=2, ElemSize=8, Stride=87, Buffer=1041(Size 21489) Attrib[1]: Ptr=004B, Type=GL_FLOAT, Size=3, ElemSize=12, Stride=87, Buffer=1041(Size 21489) Attrib[4]: Ptr=0048, Type=GL_UNSIGNED_BYTE, Size=3, ElemSize=3, Stride=87, Buffer=1041(Size 21489) Attrib[14]: Ptr=0024, Type=GL_FLOAT, Size=3, ElemSize=12, Stride=87, Buffer=1041(Size 21489) Attrib[15]: Ptr=0018, Type=GL_FLOAT, Size=3, ElemSize=12, Stride=87, Buffer=1041(Size 21489) The Attrib[1] array is float[3] at offset(Ptr) 75 (0x4B) bytes. Also note that the stride between vertices is 87 bytes so clearly there's no regard to 4-byte float alignment. I kind of wish the spec had language saying glVertexPointer, etc. generated GL_INVALID_OPERATION if the stride or offset didn't meet alignment restrictions. Too late now. We could add some debug code in Mesa to warn on unaligned arrays too. How about this: I could modify the piglit test to generate PIGLIT_WARN instead of FAIL if there's a rendering failure. I can also mention this Flockers issue in the comments in the piglit test for future reference. NVIDIA's driver handles all the cases in the piglit test (and it renders Flockers correctly). -Brian On 06/22/2015 07:26 AM, Brian Paul wrote: Thanks for pointing that out, Chris. I had forgotten about that language. This test came about after fixing a bug in our driver when running the game Flockers, from Steam. It has a few vertex arrays with unusual layout. I'll have to go back and double-check the data type. Maybe it was ubyte instead of float... -Brian On 06/21/2015 03:41 AM, Chris Forbes wrote: Jose, I believe that's exactly what it's saying, yes. - Chris On Sun, Jun 21, 2015 at 8:00 PM, Jose Fonseca wrote: It depends on the platform. But x86 doesn't require floats to be 4-byte aligned. https://urldefense.proofpoint.com/v2/url?u=https-3A__en.wikipedia.org_wiki_Data-5Fstructure-5Falignment-23x86&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=hoZapaHEiAtCP0Hl8wUx6-p35QL0Xat2wQTeFYWssvk&s=udFG-s3z0zheKxzbzEgjB4bmtZ0DLIn3FVJLIpf8L2E&e= I don't know about other platforms though -- it might be indeed necessary to skip or be less strict on non-x86 platforms. I'm having trouble interpreting the "additional base-level requirement" means. Is it saying that a an offset to a float (4bytes), must be 4bytes aligned, regardless the platform can deal with non- 4-byte aligned floats? Jose From: Piglit on behalf of Chris Forbes Sent: 20 June 2015 11:32 To: Brian Paul Cc: piglit@lists.freedesktop.org Subject: Re: [Piglit] [PATCH] gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides Brian, Doesn't this test run afoul of the alignment requirements? The OpenGL 1.5 specification, page 33 [first version in which this is introduced; equivalent language exists in later spec versions] says: "Clients must align data elements consistent with the requirements of the client platform, with an additional base-level requirement that an offset within a buffer to a datum comprising N basic machine units be a multiple of N." Non-naturally-aligned float attributes sourced from a buffer object would appear to violate this rule - or have I misunderstood? - Chris On Sat, Jun 20, 2015 at 2:39 AM, Brian Paul wrote: The draw-vertices tests exercises unusual vertex sizes and strides, but not with interleaved arrays. This test creates a VBO with interleaved vertex positions and colors. The colors are positioned at offsets like 9, 10 and 11 bytes from the start of the vertex. Unusual strides between vertices are tested too. Exercises a bug in Gallium's u_vbuf code where the driver was passed pipe_vertex_element::src_offset values which weren't a multiple of four, even when PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY returned 1. --- tests/all.py | 1 + tests/spec/gl-1.5/CMakeLists.gl.txt | 1 + tests/spec/gl-1.5/vertex-buffer-offsets.c | 124 ++ 3 files changed, 126 insertions(+) create mode 100644 tests/spec/gl-1.5/vertex-buffer-offsets.c diff --git a/tests/all.py b/tests/all.py index ad4c494..ee11be6 100755 --- a/tests/all.py +++ b/tests/all.py @@ -1042,6 +1042,7 @@ with profile.group_manager( 'normal3b3s-invariance-byte', run_concurrent=False) g(['gl-1.5-normal3b3s-invariance', 'GL_SHORT'], 'normal3b3s-invariance-short', run_co
Re: [Piglit] [PATCH] gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides
Thanks for pointing that out, Chris. I had forgotten about that language. This test came about after fixing a bug in our driver when running the game Flockers, from Steam. It has a few vertex arrays with unusual layout. I'll have to go back and double-check the data type. Maybe it was ubyte instead of float... -Brian On 06/21/2015 03:41 AM, Chris Forbes wrote: Jose, I believe that's exactly what it's saying, yes. - Chris On Sun, Jun 21, 2015 at 8:00 PM, Jose Fonseca wrote: It depends on the platform. But x86 doesn't require floats to be 4-byte aligned. https://urldefense.proofpoint.com/v2/url?u=https-3A__en.wikipedia.org_wiki_Data-5Fstructure-5Falignment-23x86&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=hoZapaHEiAtCP0Hl8wUx6-p35QL0Xat2wQTeFYWssvk&s=udFG-s3z0zheKxzbzEgjB4bmtZ0DLIn3FVJLIpf8L2E&e= I don't know about other platforms though -- it might be indeed necessary to skip or be less strict on non-x86 platforms. I'm having trouble interpreting the "additional base-level requirement" means. Is it saying that a an offset to a float (4bytes), must be 4bytes aligned, regardless the platform can deal with non- 4-byte aligned floats? Jose From: Piglit on behalf of Chris Forbes Sent: 20 June 2015 11:32 To: Brian Paul Cc: piglit@lists.freedesktop.org Subject: Re: [Piglit] [PATCH] gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides Brian, Doesn't this test run afoul of the alignment requirements? The OpenGL 1.5 specification, page 33 [first version in which this is introduced; equivalent language exists in later spec versions] says: "Clients must align data elements consistent with the requirements of the client platform, with an additional base-level requirement that an offset within a buffer to a datum comprising N basic machine units be a multiple of N." Non-naturally-aligned float attributes sourced from a buffer object would appear to violate this rule - or have I misunderstood? - Chris On Sat, Jun 20, 2015 at 2:39 AM, Brian Paul wrote: The draw-vertices tests exercises unusual vertex sizes and strides, but not with interleaved arrays. This test creates a VBO with interleaved vertex positions and colors. The colors are positioned at offsets like 9, 10 and 11 bytes from the start of the vertex. Unusual strides between vertices are tested too. Exercises a bug in Gallium's u_vbuf code where the driver was passed pipe_vertex_element::src_offset values which weren't a multiple of four, even when PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY returned 1. --- tests/all.py | 1 + tests/spec/gl-1.5/CMakeLists.gl.txt | 1 + tests/spec/gl-1.5/vertex-buffer-offsets.c | 124 ++ 3 files changed, 126 insertions(+) create mode 100644 tests/spec/gl-1.5/vertex-buffer-offsets.c diff --git a/tests/all.py b/tests/all.py index ad4c494..ee11be6 100755 --- a/tests/all.py +++ b/tests/all.py @@ -1042,6 +1042,7 @@ with profile.group_manager( 'normal3b3s-invariance-byte', run_concurrent=False) g(['gl-1.5-normal3b3s-invariance', 'GL_SHORT'], 'normal3b3s-invariance-short', run_concurrent=False) +g(['gl-1.5-vertex-buffer-offsets'], 'vertex-buffer-offsets') with profile.group_manager( PiglitGLTest, diff --git a/tests/spec/gl-1.5/CMakeLists.gl.txt b/tests/spec/gl-1.5/CMakeLists.gl.txt index 8dcd95d..f10c6cb 100644 --- a/tests/spec/gl-1.5/CMakeLists.gl.txt +++ b/tests/spec/gl-1.5/CMakeLists.gl.txt @@ -10,5 +10,6 @@ link_libraries ( ) piglit_add_executable (gl-1.5-normal3b3s-invariance normal3b3s-invariance.c) +piglit_add_executable (gl-1.5-vertex-buffer-offsets vertex-buffer-offsets.c) # vim: ft=cmake: diff --git a/tests/spec/gl-1.5/vertex-buffer-offsets.c b/tests/spec/gl-1.5/vertex-buffer-offsets.c new file mode 100644 index 000..6d58331 --- /dev/null +++ b/tests/spec/gl-1.5/vertex-buffer-offsets.c @@ -0,0 +1,124 @@ +/* + * Copyright 2015 VMware, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDER
[Piglit] Re-run filtered tests results
On 11/06/15 10:04, Emil Velikov wrote:
On 10 June 2015 at 23:56, Rob Clark wrote:
On Wed, Jun 10, 2015 at 7:28 PM, Emil Velikov wrote:
On 5 June 2015 at 22:08, Rob Clark wrote:
so, maybe a bit off topic (and maybe doesn't really help with the
whole finding a mentor thing).. but a sort of wish-list thing for
piglit that I've had in the back of my head is something like "tig"
for piglit. I suppose it doesn't have to necessarily be a curses
based UI, it could be gui.. but a lot of times while I find myself
searching through results.json to find cmdline to re-run failed tests,
which is kind of time consuming. Some sort of front-end to piglit
which would let me do things like filter on test status, then see
output/results and if I want re-run the selected test(s) would be kind
of nice.
Sounds like exactly what the html test summary provides. Although I'm
not sure how many people actually use it - json seems to be more
popular nowadays :-) I even recall naggin' on Dylan when things broke
on an occasion or two.
except html is neither interactive nor fast/convenient ;-)
The use case I'm thinking of is mostly driver devel/debug where I want
to check if some updated version of a change I am working on fixed the
regressions, and that sort of thing. (Or re-run the failed tests with
some FD_MESA_DEBUG env var debug flag.) So ability to re-run
individual tests (or group of tests based on filtered result status,
etc) is what I'm looking for. Versus digging through .json or html to
find individual cmdlines for some 100's of tests ;-)
I see. I was fortunate enough to rerun only 2-3 regressing tests,
rather than 100's, so html was convenient enough :) The "rerun all
{failing,...} tests" does sound like a useful feature.
-Emil
I've moved this to its own thread.
Rob I came across this patch by Dylan in patchwork from last year. It
doesn't have everything you want but is a step in the right direction.
Dylan if your still looking to add this you might want to also document
it in the readme file as its such a useful feature.
https://patchwork.freedesktop.org/patch/30542/
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] Weekly 10 Picks from Patchwork for review and friendly reminder to clean out your old patches
On Sun, 21 Jun 2015 23:16:40 +0200 Marek Olšák wrote: > On Fri, Jun 19, 2015 at 10:12 PM, Jose Fonseca wrote: > > On 19/06/15 20:45, Ilia Mirkin wrote: > >> > >> On Fri, Jun 19, 2015 at 3:39 PM, Jose Fonseca wrote: > >>> > >>> On 19/06/15 13:32, Timothy Arceri wrote: > > > Hi all, > > Unfortunately since its introduction patchwork hasn't seen a lot of love > in the Piglit and Mesa projects so I thought I'd try something out to > bring it out of the shadows and into the limelight. > > The idea is simple we have many useful but long forgotten patches > sitting on the mailing list that would serve us much better sitting in > the git repo, so once a week I (or anyone else that wants to help out) > would pick 10 seemingly random older patches that could do with a > review/update/etc. > > I'm hoping this will help with both clearing out the backlog of patches > and getting people thinking about patchwork. > > I'm interested in feedback on what people think about this idea. > >>> > >>> > >>> > >>> Patchwork seems to fail to recognize submited patches. Eg. one of my > >>> patches is https://patchwork.freedesktop.org/patch/51379/ but it has been > >>> commited on > >>> > >>> http://cgit.freedesktop.org/piglit/commit/?id=540972b46e51ee1d4acbb3757b731a066e2b6ba5 > >>> > >>> Why is that? > >> > >> > >> It's very strict about matching patches. The diff has to be identical. > >> Which it often isn't if you made minor changes, or rebased, or > >> whatever. > > > > > > Without a bit of fuzzy matching I'm afraid this looks a bit hopeless to me: > > > > I believe the bulk of the patches are committed, and only a few is > > forgotten. Looking at the patchwork backlog it's fair to say a large > > portion of those committed don't get detected due to small changes. So the > > end result is that developers have to click through and babysit the bulk of > > their changes in patchwork, so that the few truly forgotten patches get to > > stand out? > > > > I don't think this will ever going to work. There's no incentive in the > > system for the most prolific developers to spend so much of their time, for > > the sake of the occasional contributor. The patchwork system seems bound to > > echo what happens on the mailing list: their patches get to be lost twice... > > I couldn't agree more. > > Every day, I have to decide what the best way to spend my time is. And > patchwork is not it. There are so many patches in patchwork I don't > care about that it's tedious to find ones that I do care about. I bet most people think like that, which means most people just ignore Patchwork, the lists in Patchwork get out of date, which makes you think its far too much effort to deal with Patchwork. *shrug* > I keep track of patches and issues that interest me using the gmail > "stars". That has worked very well for me and it helps me to never > forget anything. I do ignore a lot of emails though, including 99% of > the piglit mailing list. > > If you want to get my attention, Cc me or mention my first name. That is all cool for your personal needs, and assuming people know to ping you. But do you ever review patches not explicitly pointed to you (would you want to, easily)? My experience with Patchwork is with Wayland/Weston, which surely has less traffic than Mesa. Before Patchwork, the only record of patches needing review was unread email on the mailing list. That doesn't work, because for anyone to see a status change to a patch, you have to read the replies. There is enough traffic to make that infeasible even for Wayland/Weston. Everyone has to do that on their own as there is no common tracking. And so most people can't bother. At that time, the only tracker of patches needing review was my personal email folder, which obviously doesn't scale at all and put an immense stress on me personally. Getting Patchwork up *and used* for Wayland/Weston was a huge improvement over nothing. Now we actually know what hasn't been reviewed and what are the latest revisions of patches so you don't go reviewing something that was superseded. Patchwork is not perfect nor effortless, but while waiting for the perfect infrastructure to appear, it helps a lot if you don't let it rot. I think I can provide project admin rights for people in Patchwork, so they can update the patch status of anyone's patches. I've never done it for Mesa or Piglit yet, but for Wayland yes. Feel free to ask me, I'll see about it when I can. Btw. another cave-eat with Patchwork's git hook for marking patches Accepted is that it marks the first submission of a patch. If the patch has been re-sent as is, the following copies are not updated. Paying attention to those git-push messages helps you to keep Patchwork clean, and date seems to be a nice key for finding a particular patch for manual update. Thanks, pq ___ Piglit mailing list
