[Mesa-dev] [PATCH 1/2] bin/get-pick-list.sh: rework handing of sha nominations
Emil Velikov
emil.l.velikov at gmail.com
Tue Dec 18 10:47:28 UTC 2018
On Mon, 17 Dec 2018 at 22:35, Andres Gomez <agomez at igalia.com> wrote:
>
> On Mon, 2018-12-17 at 18:36 +0000, Emil Velikov wrote:
> > On Mon, 17 Dec 2018 at 18:14, Andres Gomez <agomez at igalia.com> wrote:
> > >
> > > On Mon, 2018-12-17 at 16:43 +0000, Emil Velikov wrote:
> > > > Currently our is_sha_nomination does:
> > > > - folds any whitespace, attempting to extract sha-like information
> > > > - checks that at least one of the shas has landed
> > > >
> > > > Split it in two and do sha-like validation first.
> > > >
> > > > This way, commits with mesa-stable and sha nominations will feature the
> > > > fixes/revert/etc instead of stable (a) or will be omitted if not
> > > > applicable for the respective branch (b).
> > > >
> > > > Misc examples from 18.3
> > > >
> > > > (a)
> > > > -[ stable ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> > > > +[ fixes ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> > > >
> > > > (b)
> > > > -[ stable ] 9a7b3199037 anv/query: flush render target before copying results
> > > >
> > > > CC: Andres Gomez <agomez at igalia.com>
> > > > CC: Juan A. Suarez <jasuarez at igalia.com>
> > > > CC: Dylan Baker <dylan at pnwbakers.com>
> > > > CC: mesa-stable at lists.freedesktop.org
> > > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > > > ---
> > > > Juan I've noticed that you've been experiencing the above annoyance for
> > > > a while. Having less false-positives should ease things up a bit :-)
> > > > ---
> > > > bin/get-pick-list.sh | 46 +++++++++++++++++++++++++++++---------------
> > > > 1 file changed, 30 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/bin/get-pick-list.sh b/bin/get-pick-list.sh
> > > > index 9f9cbc44026..08a783f35a8 100755
> > > > --- a/bin/get-pick-list.sh
> > > > +++ b/bin/get-pick-list.sh
> > > > @@ -21,32 +21,36 @@ is_typod_nomination()
> > > > git show --summary "$1" | grep -q -i -o "CC:.*mesa-dev"
> > > > }
> > > >
> > > > +fixes=
> > > > +
> > >
> > > Splitting in 2 functions for which we need now a global variable is not
> > > very nice. Anyway, let's not make this more complicated than needed.
> > >
> >
> > Wasn't too happy about it either. As you said - I've decided to go
> > with the simpler solution.
> >
> > > > # Helper to handle various mistypos of the fixes tag.
> > > > # The tag string itself is passed as argument and normalised within.
> > > > +#
> > > > +# Resulting string in the global variable "fixes" and contains entries
> > > > +# in the form "fixes:$sha"
> > > > is_sha_nomination()
> > > > {
> > > > fixes=`git show --pretty=medium -s $1 | tr -d "\n" | \
> > > > sed -e 's/'"$2"'/\nfixes:/Ig' | \
> > > > grep -Eo 'fixes:[a-f0-9]{8,40}'`
> > > >
> > > > - fixes_count=`echo "$fixes" | wc -l`
> > > > + fixes_count=`echo "$fixes" | grep "fixes:" | wc -l`
> > >
> > > Why is the extra "grep" needed here?
> > >
> >
> > Commits that include "fixes $some_test" will result in fixes="".
> > Thus wc -l will return 1
>
> Argh!
>
> Well, this is more a consequence of wc counting newline characters and
> echo adding a new one at the end, even when not printing anything.
> Let's grep again, then ☹
>
> > > > if test $fixes_count -eq 0; then
> > > > - return 0
> > > > + return 1
> > >
> > > Ugh! Right.
> > >
> > > > fi
> > > > + return 0
> > > > +}
> > > > +
> > > > +# Checks if at least one of offending commits, listed in the global
> > > > +# "fixes", is in branch.
> > > > +sha_in_range()
> > > > +{
> > > > + fixes_count=`echo "$fixes" | grep "fixes:" | wc -l`
> > >
> > > Ditto.
> > >
> > > > while test $fixes_count -gt 0; do
> > > > # Treat only the current line
> > > > id=`echo "$fixes" | tail -n $fixes_count | head -n 1 | cut -d : -f 2`
> > > > fixes_count=$(($fixes_count-1))
> > > >
> > > > - # Bail out if we cannot find suitable id.
> > > > - # Any specific validation the $id is valid and not some junk, is
> > > > - # implied with the follow up code
> > > > - if test "x$id" = x; then
> > > > - continue
> > > > - fi
> > > > -
> > > > - #Check if the offending commit is in branch.
> > > > -
> > >
> > > Was this never needed in the first place? Feels right to remove it
> > > since $fixes should have some content by now, but I wonder how this got
> > > here in the first place.
> > >
> >
> > Left-over from the old standalone script. Copied a bit too much :-(
> >
> > > > # Be that cherry-picked ...
> > > > # ... or landed before the branchpoint.
> > > > if grep -q ^$id already_picked ||
> > > > @@ -103,20 +107,30 @@ do
> > > > continue
> > > > fi
> > > >
> > > > - if is_stable_nomination "$sha"; then
> > > > - tag=stable
> > > > - elif is_typod_nomination "$sha"; then
> > > > - tag=typod
> > > > - elif is_fixes_nomination "$sha"; then
> > > > + if is_fixes_nomination "$sha"; then
> > > > tag=fixes
> > > > elif is_brokenby_nomination "$sha"; then
> > > > tag=brokenby
> > > > elif is_revert_nomination "$sha"; then
> > > > tag=revert
> > > > + elif is_stable_nomination "$sha"; then
> > > > + tag=stable
> > > > + elif is_typod_nomination "$sha"; then
> > > > + tag=typod
> > > > else
> > > > continue
> > > > fi
> > > >
> > > > + case "$tag" in
> > > > + fixes | brokenby | revert )
> > > > + if ! sha_in_range; then
> > > > + continue
> > > > + fi
> > > > + ;;
> > > > + * )
> > > > + ;;
> > > > + esac
> > > > +
> > > > printf "[ %8s ] " "$tag"
> > > > git --no-pager show --summary --oneline $sha
> > > > done
> > >
> > > I'm not sure we are gaining something with the splitting.
> > >
> > > Maybe I'm not understanding correctly the patch but it seems to me that
> > > every successful "is_sha_nomination" is followed by a "sha_in_range"
> > > call. Hence, we are only trading splitting into 2 functions by having a
> > > global variable (!)
> > >
> >
> > As-is one gets false-positives, for commits like 9a7b3199037 mentioned above.
> > As outlined, is_sha_nomination does two separate things. Thus even
> > though 9a7b3199037 contains a valid offending sha, we "ignore" that
> > and end up flagging the commit.
> >
> > To better illustrate what that here is an before/after example for
> > mesa-18.2.1 (+ cherry-pick
> > 559c32d2412b2ea602bb59aa61da75403d01a872~1..adbdfc6666052d604a97009d736b6dee957908a0)
> >
> > --- old 2018-12-17 18:31:06.023967483 +0000
> > +++ new 2018-12-17 18:30:37.963966341 +0000
> > @@ -9,14 +9,14 @@
> > [ fixes ] 01fa76b7072 nvc0: Update counter reading shaders to new
> > NVC0_CB_AUX_MP_INFO
> > [ fixes ] c95dd966c43 docs: Update FAQ with respect to s3tc support
> > [ fixes ] b473fcc9a39 nvc0: fix bindless multisampled images on Maxwell+
> > -[ stable ] 3e7b5e5db2f radeon/uvd: use bitstream coded number for
> > symbols of Huffman tables
> > +[ fixes ] 3e7b5e5db2f radeon/uvd: use bitstream coded number for
> > symbols of Huffman tables
> > [ fixes ] bde3102c0dc vulkan/wsi/display: check if
> > wsi_swapchain_init() succeeded
> > -[ stable ] ec1fcf92ae7 radv: only emit ZPASS_DONE for timestamp
> > queries on gfx queues
> > +[ fixes ] ec1fcf92ae7 radv: only emit ZPASS_DONE for timestamp
> > queries on gfx queues
> > [ stable ] 7ee5e5e239a st/nine: Clamp RCP when 0*inf!=0
> > [ stable ] dcfde02bb0f st/nine: Avoid redundant SetCursorPos calls
> > [ stable ] 7ae2509ce06 st/nine: Increase maximum number of temp registers
> > [ stable ] 0d495bec25b radeonsi: NaN should pass kill_if
> > -[ stable ] dd333c66bdc vulkan: Disable randr lease for libxcb < 1.13
> > +[ fixes ] dd333c66bdc vulkan: Disable randr lease for libxcb < 1.13
> > [ fixes ] 7e7959fcb76 intel/fs: Fix a typo in need_matching_subreg_offset
> > [ stable ] bfc89c668e2 nir/cf: Remove phi sources if needed in
> > nir_handle_add_jump
> > [ fixes ] 00f385e6d45 nir/from_ssa: Don't rewrite derefs
> > destinations to registers
> > @@ -44,7 +44,7 @@
> > [ stable ] d179312b53d radv: add a workaround for a VGT hang with
> > prim restart and strips
> > [ stable ] d76c2774219 st/va: use provided sizes and coords for vlVaGetImage
> > [ fixes ] cc33621e3b8 r600/sb: Fix constant-logical-operand warning.
> > -[ stable ] ca83d51cfb1 ac/nir: Use context-specific LLVM types
> > +[ fixes ] ca83d51cfb1 ac/nir: Use context-specific LLVM types
> > [ fixes ] c20ba1be184 loader/dri3: Also wait for front buffer
> > fence if we triggered it
> > [ stable ] e71a87775e4 radv: fix check for perftest options size
> > [ stable ] 06bf56725db radeonsi: Bump number of allowed global buffers to 32
> > @@ -61,7 +61,6 @@
> > [ fixes ] 5bcf479524b intel/blorp: Define the clear value bounds
> > for HiZ clears
> > [ revert ] d6be0b5556c scons/svga: remove opt from the list of
> > valid build types
> > [ stable ] b6b2b27809b blorp: Emit a dummy 3DSTATE_WM prior to
> > 3DSTATE_WM_HZ_OP
> > -[ stable ] aa02d7e8781 Revert "anv/skylake: disable
> > ForceThreadDispatchEnable"
> > [ fixes ] a61952e7374 freedreno: don't flush when new and old pfb
> > is identical
> > [ fixes ] 98e7c3e7a72 svga: add missing meson build dependency
> > [ fixes ] 1df0c1e8fbd clover: add missing meson build dependency
> > @@ -75,15 +74,14 @@
> > [ stable ] ae8e81b0e30 intel/tools: include stdarg.h in error2aub
> > [ fixes ] 7652931d33b meson: link gallium nine with pthreads
> > [ fixes ] 64a9ed8848e r600/sb: Fix constant logical operand in assert.
> > -[ stable ] b1b2dd06a7b radv: add missing TFB queries support to
> > CmdCopyQueryPoolsResults()
> > [ fixes ] d515ded4d95 wsi/wayland: only finish() a successfully
> > init()ed display
> > [ fixes ] 04298a2f244 st/va: fix incorrect use of resource_destroy
> > [ fixes ] b3ade653879 egl/glvnd: correctly report errors when
> > vendor cannot be found
> > -[ stable ] 55af17ffed2 wayland/egl: Resize EGL surface on update
> > buffer for swrast
> > +[ fixes ] 55af17ffed2 wayland/egl: Resize EGL surface on update
> > buffer for swrast
> > [ fixes ] 421fa01d64d anv/android: mark gralloc allocated BOs as external
> > [ stable ] 0a0aa2ba6c3 radv: disable conditional rendering for
> > vkCmdCopyQueryPoolResults()
> > [ stable ] 0dcd99c6870 radv: only expose
> > VK_SUBGROUP_FEATURE_ARITHMETIC_BIT for VI+
> > -[ stable ] 10598c9667a st/nine: fix stack corruption due to ABI mismatch
> > +[ fixes ] 10598c9667a st/nine: fix stack corruption due to ABI mismatch
> > [ fixes ] 9dd737bb029 nir: add glsl_type_is_integer() helper
> > [ fixes ] a068958692c nir: don't pack varyings ints with floats unless flat
> > [ fixes ] 9c2a95b2986 meson: Don't set -Wall
> > @@ -115,7 +113,7 @@
> > [ fixes ] cc0bc76a382 vc4: Make sure we make ro scanout resources
> > for create_with_modifiers.
> > [ fixes ] 1c56d211563 egl/dri: fix error value with unknown drm format
> > [ stable ] 84445a86d19 travis: drop unneeded x11proto-xf86vidmode-dev
> > -[ stable ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> > +[ fixes ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> > [ fixes ] 982e012b3ac travis: adding missing x11-xcb for meson+vulkan
> > [ fixes ] b787dcf57b7 i965/batch: avoid reverting batch buffer if
> > saved state is an empty
> > [ stable ] ea9f95e2a67 radeonsi: go back to using bottom-of-pipe
> > for beginning of TIME_ELAPSED
> > @@ -142,13 +140,11 @@
> > [ stable ] 4f74580d303 st/xvmc: Add X11 include path.
> > [ fixes ] fc0139d2833 nv50,nvc0: Fix gallium nine regression
> > regarding sampler bindings
> > [ fixes ] 9401a2f2e64 amd/vulkan: meson build - use radv_deps for
> > libvulkan_radeon
> > -[ stable ] 017199d2d2e mesa: Revert INTEL_fragment_shader_ordering support
> > [ fixes ] 51091b3e1f2 radv/android: Mark android WSI image as shareable.
> > [ fixes ] 3bf48741e12 radv/android: Use buffer metadata to
> > determine scanout compat.
> > [ fixes ] 1363a47c9c4 radv: use 3d shader for gfx9 copies if dst is 3d
> > [ fixes ] 824cfc1ee5e radv: rework the TC-compat HTILE hardware
> > bug with COND_EXEC
> > [ fixes ] c1b6cb068c4 radv: Flush before vkCmdWriteTimestamp() if needed
> > -[ stable ] 9a7b3199037 anv/query: flush render target before copying results
> > [ stable ] c0ac038c97b gallium: Constify drisw_loader_funcs struct
> > [ fixes ] 63c0916ada7 drisw: Use separate drisw_loader_funcs for shm
> > [ fixes ] 3bd73d31a86 v3d: Fix a leak of the transfer helper on
> > screen destroy.
> >
> > As you can see some bogus stable nominations are removed, et al. That
> > is not possible without the is_sha_nomination rework.
>
> I see that, but it is a consequence of the reordering of the checks,
> not of the rework ... or maybe I'm too blind.
>
I agree looking at the diff alone, is quite odd. Let's take
9a7b3199037 and consider only a reorder.
- in is_sha_nomination
- offending commit is extracted correctly
- offending commit hasn't landed in before the branchpoint
- .... or after the branchpoint (as cherry-pick)
- hence is_sha_nomination returns false
- fallback to mesa-stable
- which leads to the false positive
> > > Additionally, in the second patch of the series we are adding a warning
> > > that, in case of having a single function, could be placed in the same
> > > while loop, instead of having now 2 loops (?)
> > >
> >
> > In about ~90% of the case, there's a single fixes tag. So I wouldn't
> > worry about that.
>
> That's true.
>
>
> Anyway, I'm fine with it. This series is:
>
> Acked-by: Andres Gomez <agomez at igalia.com>
>
Thank you.
To be on the safe side I'll double-check against 18.1 (currently I've
tried 18.2 and 18.3)
-Emil
More information about the mesa-dev
mailing list