diff mbox

[i-g-t,3/3] lib: Reduce dependency on glib

Message ID 1506682319-16165-4-git-send-email-petri.latvala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Petri Latvala Sept. 29, 2017, 10:51 a.m. UTC 
In commit ebd6eb69f57b ("Make igtrc configuration common, with
configurable suspend/resume delay") .igtrc handling was moved to
igt_core from igt_chamelium. That made everything in IGT depend on
GLIB by accident.

In short, igt_core.h declared a variable of type GKeyFile*, requiring
glib.h. Everything that tried to #include igt_core.h required glib.h
to be available, by use of GLIB_CFLAGS. This "worked" so far because
CAIRO_CFLAGS contained GLIB_CFLAGS.

As the variable is only used by other stuff in lib/, stuff it in its
own header file to leave igt_core.h without a dependency to glib.h.

Also add a couple of missing #ifdef HAVE_GLIBs around.

Reported-by: Felipe De Jesus Ruiz Garcia <felipe.de.jesus.ruiz.garcia@intel.com>
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/Makefile.sources |  1 +
 lib/igt_chamelium.c  |  1 +
 lib/igt_core.c       |  3 +++
 lib/igt_core.h       |  2 --
 lib/igt_rc.h         | 34 ++++++++++++++++++++++++++++++++++
 lib/meson.build      |  1 +
 6 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 lib/igt_rc.h

Comments

Arkadiusz Hiler Sept. 29, 2017, 12:09 p.m. UTC | #1 
On Fri, Sep 29, 2017 at 01:51:59PM +0300, Petri Latvala wrote:
> In commit ebd6eb69f57b ("Make igtrc configuration common, with
> configurable suspend/resume delay") .igtrc handling was moved to
> igt_core from igt_chamelium. That made everything in IGT depend on
> GLIB by accident.
> 
> In short, igt_core.h declared a variable of type GKeyFile*, requiring
> glib.h. Everything that tried to #include igt_core.h required glib.h
> to be available, by use of GLIB_CFLAGS. This "worked" so far because
> CAIRO_CFLAGS contained GLIB_CFLAGS.
> 
> As the variable is only used by other stuff in lib/, stuff it in its
> own header file to leave igt_core.h without a dependency to glib.h.
> 
> Also add a couple of missing #ifdef HAVE_GLIBs around.
> 
> Reported-by: Felipe De Jesus Ruiz Garcia <felipe.de.jesus.ruiz.garcia@intel.com>
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

The whole igtrc implementation has plenty of other issues (using asserts
in a wrong scope, etc).

I have a bigger rework on my TODO list...
Daniel Vetter Oct. 2, 2017, 8:52 a.m. UTC | #2 
On Fri, Sep 29, 2017 at 03:09:02PM +0300, Arkadiusz Hiler wrote:
> On Fri, Sep 29, 2017 at 01:51:59PM +0300, Petri Latvala wrote:
> > In commit ebd6eb69f57b ("Make igtrc configuration common, with
> > configurable suspend/resume delay") .igtrc handling was moved to
> > igt_core from igt_chamelium. That made everything in IGT depend on
> > GLIB by accident.
> > 
> > In short, igt_core.h declared a variable of type GKeyFile*, requiring
> > glib.h. Everything that tried to #include igt_core.h required glib.h
> > to be available, by use of GLIB_CFLAGS. This "worked" so far because
> > CAIRO_CFLAGS contained GLIB_CFLAGS.
> > 
> > As the variable is only used by other stuff in lib/, stuff it in its
> > own header file to leave igt_core.h without a dependency to glib.h.
> > 
> > Also add a couple of missing #ifdef HAVE_GLIBs around.
> > 
> > Reported-by: Felipe De Jesus Ruiz Garcia <felipe.de.jesus.ruiz.garcia@intel.com>
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> The whole igtrc implementation has plenty of other issues (using asserts
> in a wrong scope, etc).
> 
> I have a bigger rework on my TODO list...

Imo biggest is inversion of control, the core igtrc does all the parsing
for all the other bits in igt, which means we leak all the submodule stuff
into igt_core.c. Which isn't cool.

Is there a JIRA or similar for this already where I could bikeshed?
-Daniel
diff mbox

Patch

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 53fdb54c..e851f62b 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -20,6 +20,7 @@  lib_source_list =	 	\
 	igt_primes.h		\
 	igt_rand.c		\
 	igt_rand.h		\
+	igt_rc.h		\
 	igt_stats.c		\
 	igt_stats.h		\
 	igt_sysfs.c		\
diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index fb250366..b25855a4 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -40,6 +40,7 @@ 
 #include "igt_aux.h"
 #include "igt_kms.h"
 #include "igt_frame.h"
+#include "igt_rc.h"
 
 /**
  * SECTION:igt_chamelium
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 47b4682d..950ea9b0 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -55,7 +55,9 @@ 
 #include <limits.h>
 #include <locale.h>
 #include <uwildmat/uwildmat.h>
+#ifdef HAVE_GLIB
 #include <glib.h>
+#endif
 
 #include "drmtest.h"
 #include "intel_chipset.h"
@@ -67,6 +69,7 @@ 
 #include "igt_core.h"
 #include "igt_aux.h"
 #include "igt_sysfs.h"
+#include "igt_rc.h"
 
 #ifdef HAVE_LIBGEN_H
 #include <libgen.h>   /* for basename() on Solaris */
diff --git a/lib/igt_core.h b/lib/igt_core.h
index c90ae2de..f8543d65 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -40,7 +40,6 @@ 
 #include <stdarg.h>
 #include <getopt.h>
 #include <unistd.h>
-#include <glib.h>
 
 #ifndef IGT_LOG_DOMAIN
 #define IGT_LOG_DOMAIN (NULL)
@@ -49,7 +48,6 @@ 
 
 extern const char* __igt_test_description __attribute__((weak));
 extern bool __igt_plain_output;
-extern GKeyFile *igt_key_file;
 extern char *igt_frame_dump_path;
 
 /**
diff --git a/lib/igt_rc.h b/lib/igt_rc.h
new file mode 100644
index 00000000..d7a314c5
--- /dev/null
+++ b/lib/igt_rc.h
@@ -0,0 +1,34 @@ 
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * 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.
+ */
+
+
+#ifndef IGT_RC_H
+#define IGT_RC_H
+
+#ifdef HAVE_GLIB
+#include <glib.h>
+
+extern GKeyFile *igt_key_file;
+
+#endif /* HAVE_GLIB */
+#endif /* IGT_RC_H */
diff --git a/lib/meson.build b/lib/meson.build
index 203be520..54ff4f9b 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -13,6 +13,7 @@  lib_headers = [
 	'igt_gvt.h',
 	'igt_primes.h',
 	'igt_rand.h',
+	'igt_rc.h',
 	'igt_stats.h',
 	'igt_sysfs.h',
 	'igt_x86.h',