public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t 0/3] Convert sh scripts to C variants.
Date: Mon, 24 Oct 2016 10:49:21 +0200	[thread overview]
Message-ID: <20161024084921.GI20761@phenom.ffwll.local> (raw)
In-Reply-To: <20161024084614.GH20761@phenom.ffwll.local>

On Mon, Oct 24, 2016 at 10:46:14AM +0200, Daniel Vetter wrote:
> On Fri, Oct 21, 2016 at 10:38:17AM +0300, Joonas Lahtinen wrote:
> > On pe, 2016-10-21 at 00:00 +0300, Jani Nikula wrote:
> > > > On Thu, 20 Oct 2016, Marius Vlad <marius.c.vlad@intel.com> wrote:
> > > > 
> > > > This series adds some library support to help converting sh
> > > > scripts to C version. Converted drv_module_reload_basic and
> > > > kms_sysfs_edid_timing.
> > > 
> > > > 
> > > >  18 files changed, 600 insertions(+), 180 deletions(-)
> > > 
> > > Someone please justify this, plus pulling in two new dependencies. I can
> > > think of a thing or two, but it needs to be in the commit messages. And
> > > I'm not convinced by the justification I came up with.
> 
> - Be able to reuse the subtest/logging/whatever else igt stuff for more
>   consistency, without having to reinvent the wheel in the bash world a
>   2nd time. Often this is also tricky (we have unit tests for igt
>   libraries for a reason). I rejected C++ as a new language for similar
>   reasons, I think getting rid of bash is useful.
> 
> - dmesg logging. Imo the piglit dmesg capturing serious sucks, it'd be
>   great to move it into igt. Reasons for that: a) in C it's much faster,
>   b) integrated with igt logging (consistent timestamps, ordering,
>   crashbox log, ...) c) we could put the filtering of dmesg next to the
>   tests, atm it's some rough filter in piglit's igt.py. c) has been one of
>   Chris' wishlist things, e.g. make underrun tests hunt for underruns
>   explicitly.
> 
> - It's a nice ramp-up task for igt, that's why I bumped it up a bit. Would
>   be fairly low-prio otherwise.
> 
> > Hmm, not sure how Daniel instructed things. Original idea was to just
> > execv the same commands as the scripts do. To get rid of the
> > interpreter differences and allow running in a minimal environment.
> 
> Yeah, I was thinking of a pretty minimal conversion to C with just lots of
> calls to system. Maybe long-term we could extract some shared code, but
> meh. Creating good libraries is a lot more work than it generally looks
> like, and code reuse for code reuse's sake is in my experience often not
> worth the hassle.

On that note, we'd probably need an igt helper to run a shell command with
full igt integration. The important bits there would be to read
stderr/stdout and feed them into igt_warn and igt_debug levels. Of course
in parallel to running the command to make sure the timestamps are
correct, and will line up with the dmesg timestampes. And long-term also
interleave with dmesg output (once we've moded that into igt proper).

That function also needs some flags to optionally ignore stderr or stdout,
which essentially would be reimplenting > /dev/null and &> /dev/null.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

      reply	other threads:[~2016-10-24  8:49 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-20 19:36 [PATCH i-g-t 0/3] Convert sh scripts to C variants Marius Vlad
2016-10-20 19:36 ` [PATCH i-g-t 1/3] lib/{igt_sysfs, igt_aux}: Make available to other users kick_fbcon() (unbind_fbcon()), and added helpers to igt_aux Marius Vlad
2016-10-20 20:09   ` Chris Wilson
2016-10-24 18:08     ` Marius Vlad
2016-10-24  8:40   ` Daniel Vetter
2016-10-26 21:02     ` Marius Vlad
2016-10-27  6:40       ` Daniel Vetter
2016-10-20 19:36 ` [PATCH i-g-t 2/3] tests/drv_module_reload_basic: Convert sh script to C version Marius Vlad
2016-10-20 19:52   ` Chris Wilson
2016-10-24 18:05     ` Marius Vlad
2016-10-24 20:34       ` Chris Wilson
2016-10-21  9:39   ` Petri Latvala
2016-10-24 18:06     ` Marius Vlad
2016-10-20 19:36 ` [PATCH i-g-t 3/3] tests/kms_sysfs_edid_timing: " Marius Vlad
2016-10-20 19:58   ` Chris Wilson
2016-10-20 21:00 ` [PATCH i-g-t 0/3] Convert sh scripts to C variants Jani Nikula
2016-10-21  7:38   ` Joonas Lahtinen
2016-10-24  8:46     ` Daniel Vetter
2016-10-24  8:49       ` Daniel Vetter [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161024084921.GI20761@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=joonas.lahtinen@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox