[PATCH master 0/2] Adding Attoparsec to Ganeti

[Michele Tartara]

This patch set adds Attoparsec to the libraries needed to build ganeti
and introduces a unittest to verify that it is actually able to support
Unicode characters.

All of this is needed in order to later use Attoparsec as the parsing
library for the future haskell data collectors for the monitoring agent.

Michele Tartara (2):
Add "attoparsec" to the required haskell packages
Added haskell unit test for Unicode parsing

Makefile.am | 2 +
configure.ac | 1 +
htest/Test/Ganeti/SimpleParser.hs | 42 +++++++++++++++++++++++++++
htest/Test/Ganeti/UnicodeParser.hs | 55 ++++++++++++++++++++++++++++++++++++
htest/test.hs | 2 +
5 files changed, 102 insertions(+), 0 deletions(-)
create mode 100644 htest/Test/Ganeti/SimpleParser.hs
create mode 100644 htest/Test/Ganeti/UnicodeParser.hs

--
1.7.7.3

[Michele Tartara]

This will be needed for the data collectors of the monitoring agent

Signed-off-by: Michele Tartara <mtar...@google.com>
---
configure.ac | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 14554a3..c54047d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -460,6 +460,7 @@ AC_GHC_PKG_REQUIRE(network)
AC_GHC_PKG_REQUIRE(mtl)
AC_GHC_PKG_REQUIRE(bytestring)
AC_GHC_PKG_REQUIRE(utf8-string)
+AC_GHC_PKG_REQUIRE(attoparsec)

# extra modules for confd functionality
HTOOLS_REGEX_PCRE=-DNO_REGEX_PCRE
--
1.7.7.3

[Michele Tartara]

Attoparsec is known to have had issues with parsing non-ASCII strings.
This test makes sure that parsing of Unicode characters works fine.

Signed-off-by: Michele Tartara <mtar...@google.com>
---

Makefile.am | 2 +

htest/Test/Ganeti/SimpleParser.hs | 42 +++++++++++++++++++++++++++
htest/Test/Ganeti/UnicodeParser.hs | 55 ++++++++++++++++++++++++++++++++++++
htest/test.hs | 2 +

4 files changed, 101 insertions(+), 0 deletions(-)

create mode 100644 htest/Test/Ganeti/SimpleParser.hs
create mode 100644 htest/Test/Ganeti/UnicodeParser.hs

diff --git a/Makefile.am b/Makefile.am
index d3103ab..b42b18b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -497,11 +497,13 @@ HS_TEST_SRCS = \
htest/Test/Ganeti/Query/Language.hs \
htest/Test/Ganeti/Query/Query.hs \
htest/Test/Ganeti/Rpc.hs \
+ htest/Test/Ganeti/SimpleParser.hs \
htest/Test/Ganeti/Ssconf.hs \
htest/Test/Ganeti/THH.hs \
htest/Test/Ganeti/TestCommon.hs \
htest/Test/Ganeti/TestHTools.hs \
htest/Test/Ganeti/TestHelper.hs \
+ htest/Test/Ganeti/UnicodeParser.hs \
htest/Test/Ganeti/Utils.hs

HS_LIBTEST_SRCS = $(HS_LIB_SRCS) $(HS_TEST_SRCS)
diff --git a/htest/Test/Ganeti/SimpleParser.hs b/htest/Test/Ganeti/SimpleParser.hs
new file mode 100644
index 0000000..2919c28
--- /dev/null
+++ b/htest/Test/Ganeti/SimpleParser.hs
@@ -0,0 +1,42 @@
+{-# LANGUAGE OverloadedStrings #-}
+
+{-| Simple parser able to split a string in two parts, name and value,
+ separated by a '=' sign -}
+
+{-
+
+Copyright (C) 2009, 2010, 2011, 2012 Google Inc.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301, USA.
+
+-}
+
+module Test.Ganeti.SimpleParser (simpleParser, name, value) where
+
+import qualified Data.Attoparsec.Text as A
+import Data.Attoparsec.Text (Parser)
+import Data.Text (unpack)
+
+data SplitString = SplitString { name::String, value::String } deriving (Show)
+
+simpleParser :: Parser SplitString
+simpleParser = do
+ n <- A.takeTill (\c -> A.isHorizontalSpace c || c == '=')
+ A.skipWhile A.isHorizontalSpace
+ _ <- A.char '='
+ A.skipWhile A.isHorizontalSpace
+ v <- A.takeTill A.isEndOfLine
+ return $ SplitString (unpack n) (unpack v)
diff --git a/htest/Test/Ganeti/UnicodeParser.hs b/htest/Test/Ganeti/UnicodeParser.hs
new file mode 100644
index 0000000..1e18976
--- /dev/null
+++ b/htest/Test/Ganeti/UnicodeParser.hs
@@ -0,0 +1,55 @@
+{-# LANGUAGE TemplateHaskell #-}
+{-| Unittests for Attoparsec support for unicode
+
+-}
+
+{-
+
+Copyright (C) 2009, 2010, 2011, 2012 Google Inc.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301, USA.
+
+-}
+
+module Test.Ganeti.UnicodeParser (testUnicode) where
+
+import Test.QuickCheck
+
+import Test.Ganeti.TestHelper
+import Test.Ganeti.TestCommon
+
+import Test.Ganeti.SimpleParser
+import qualified Data.Attoparsec.Text as A
+import Data.Text (pack)
+
+
+part1 :: String
+part1 = "äßĉ"
+
+part2 :: String
+part2 = "ðèق"
+
+prop_parserSupportsUnicode :: Property
+prop_parserSupportsUnicode =
+ let (Right split) = A.parseOnly simpleParser (Data.Text.pack $ part1 ++ " = \t" ++ part2)
+ n = name split
+ v = value split
+ in (n ==? part1) .&&. (v ==? part2)
+
+
+testSuite "Unicode"
+ [ 'prop_parserSupportsUnicode
+ ]
diff --git a/htest/test.hs b/htest/test.hs
index 5180878..f6a6922 100644
--- a/htest/test.hs
+++ b/htest/test.hs
@@ -56,6 +56,7 @@ import Test.Ganeti.Query.Query
import Test.Ganeti.Rpc
import Test.Ganeti.Ssconf
import Test.Ganeti.THH
+import Test.Ganeti.UnicodeParser
import Test.Ganeti.Utils

-- | Our default test options, overring the built-in test-framework
@@ -99,6 +100,7 @@ allTests =
, testRpc
, testSsconf
, testTHH
+ , testUnicode
, testUtils
]

--
1.7.7.3

[Iustin Pop]

On Thu, Nov 08, 2012 at 12:18:29PM +0100, Michele Tartara wrote:
> This will be needed for the data collectors of the monitoring agent

1. Since this is not available in squeeze, we can't make it a required
dependency. It should be optional, the same way we have confd.

2. You also need to document this in INSTALL and maybe devnotes.rst.

thanks,
iustin

[Iustin Pop]

On Thu, Nov 08, 2012 at 12:18:42PM +0100, Michele Tartara wrote:
> Attoparsec is known to have had issues with parsing non-ASCII strings.
> This test makes sure that parsing of Unicode characters works fine.

Hmmm. We organise test modules based on the actual modules they test.
And we don't create separate "small" modules (with just one or two
tests) if they test the same backend functionality.

Since there won't be a parser module on its own, I'd recommend just
putting this in htest/Test/Ganeti/Attoparsec.hs (to be clear that we
test attoparsec, and not our own functionality), and export from it both
simple and unicode tests.

thanks,
iustin

[Michele Tartara]

List added back

On Thu, Nov 8, 2012 at 12:33 PM, Michele Tartara <mtar...@google.com> wrote:

Ok, sorry, I still need to grasp all the details of how the codebase is organized.

I'll fix and then resend both the patches.

Thanks,

Michele

[Michele Tartara]

This patch set adds Attoparsec to the libraries optionally needed to build
Ganeti and introduces a unit test to verify that it is actually able to support

Unicode characters.

All of this is needed in order to later use Attoparsec as the parsing library

for the future Haskell data collectors of the monitoring agent.

NB: this patch set completely substitutes the previous one with the same name

Michele Tartara (2):
Add "Attoparsec" to the optional haskell packages
Added attoparsec unit test for unicode parsing

INSTALL | 13 ++++---
Makefile.am | 7 ++++
configure.ac | 34 ++++++++++++++++++
htest/Test/Ganeti/Attoparsec.hs | 72 +++++++++++++++++++++++++++++++++++++++
htest/test.hs | 7 ++++
lib/constants.py | 1 +
6 files changed, 129 insertions(+), 5 deletions(-)
create mode 100644 htest/Test/Ganeti/Attoparsec.hs

--
1.7.7.3

[Michele Tartara]

This will be needed for the data collectors of the monitoring agent.

* Detection of the library
* Creation of the appropriate variables
* Update to the installation documentation

Signed-off-by: Michele Tartara <mtar...@google.com>
---

INSTALL | 13 ++++++++-----
Makefile.am | 6 ++++++
configure.ac | 34 ++++++++++++++++++++++++++++++++++
lib/constants.py | 1 +
4 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/INSTALL b/INSTALL
index 0087e53..44088b6 100644
--- a/INSTALL
+++ b/INSTALL
@@ -168,9 +168,9 @@ Haskell optional features

Optionally, more functionality can be enabled if your build machine has
a few more Haskell libraries enabled: RAPI access to remote cluster from
-htools (``--enable-htools-rapi``) and enabling the ``ganeti-confd``
-daemon (``--enable-confd``). The list of extra dependencies for these
-is:
+htools (``--enable-htools-rapi``), the ``ganeti-confd``
+daemon (``--enable-confd``) and the monitoring agent
+(``--enable-monitoring``). The list of extra dependencies for these is:

- `curl <http://hackage.haskell.org/package/curl>`_, tested with
versions 1.3.4 and above
@@ -182,16 +182,19 @@ is:
- `hinotify <http://hackage.haskell.org/package/hinotify>`_
- `regex-pcre <http://hackage.haskell.org/package/regex-pcre>`_,
bindings for the ``pcre`` library
+- `attoparsec <http://hackage.haskell.org/package/attoparsec>`_

These libraries are available in Debian Wheezy (but not in Squeeze, with
the exception of curl), so you can use either apt::

$ apt-get install libghc-hslogger-dev libghc-crypto-dev libghc-text-dev \
- libghc-hinotify-dev libghc-regex-pcre-dev libghc-curl-dev
+ libghc-hinotify-dev libghc-regex-pcre-dev libghc-curl-dev \
+ libghc-attoparsec-dev

or ``cabal``::

- $ cabal install hslogger Crypto text hinotify regex-pcre curl
+ $ cabal install hslogger Crypto text hinotify regex-pcre curl \
+ attoparsec

to install them.

diff --git a/Makefile.am b/Makefile.am
index d6bf864..06e2e21 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -627,6 +627,7 @@ $(HS_ALL_PROGS): %: %.hs $(HS_LIBTEST_SRCS) $(HS_BUILT_SRCS) Makefile
$(GHC) --make \
$(HFLAGS) \
$(HTOOLS_NOCURL) $(HTOOLS_PARALLEL3) $(HTOOLS_REGEX_PCRE) \
+ $(NO_ATTOPARSEC) \
-osuf $(notdir $@).o -hisuf $(notdir $@).hi \
$(HEXTRA) $(HEXTRA_INT) $@
@touch "$@"
@@ -1251,6 +1252,7 @@ lib/_autoconf.py: Makefile | stamp-directories
echo "QEMUIMG_PATH = '$(QEMUIMG_PATH)'"; \
echo "HTOOLS = True"; \
echo "ENABLE_CONFD = $(ENABLE_CONFD)"; \
+ echo "ENABLE_MONITORING = $(ENABLE_MONITORING)"; \
echo "XEN_CMD = '$(XEN_CMD)'"; \
echo "ENABLE_SPLIT_QUERY = $(ENABLE_SPLIT_QUERY)"; \
} > $@
@@ -1615,6 +1617,9 @@ hs-apidoc: $(HS_BUILT_SRCS)
if [ "$(HTOOLS_REGEX_PCRE)" ]; \
then OPTGHC="$$OPTGHC --optghc=$(HTOOLS_REGEX_PCRE)"; \
fi; \
+ if [ "$(NO_ATTOPARSEC)" ]; \
+ then OPTGHC="$$OPTGHC --optghc=$(NO_ATTOPARSEC)"; \
+ fi; \
RELSRCS="$(HS_LIB_SRCS:htools/%=%) $(patsubst htools/%,%,$(filter htools/%,$(HS_BUILT_SRCS)))"; \
for file in $$RELSRCS; do \
hfile=`echo $$file|sed 's/\\.hs$$//'`.html; \
@@ -1633,6 +1638,7 @@ TAGS: $(GENERATED_FILES)
$(GHC) -e ":etags" -v0 \
$(filter-out -O -Werror,$(HFLAGS)) \
$(HTOOLS_NOCURL) $(HTOOLS_PARALLEL3) $(HTOOLS_REGEX_PCRE) \
+ $(NO_ATTOPARSEC) \
$(HS_LIBTEST_SRCS)
find . -path './lib/*.py' -o -path './scripts/gnt-*' -o \
-path './daemons/ganeti-*' -o -path './tools/*' -o \
diff --git a/configure.ac b/configure.ac
index 14554a3..a430cc8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -416,6 +416,13 @@ AC_ARG_ENABLE([confd],
[],
[enable_confd=check])

+ENABLE_MONITORING=
+AC_ARG_ENABLE([monitoring],
+ [AS_HELP_STRING([--enable-monitoring],
+ [enable the ganeti monitoring agent (default: check)])],
+ [],
+ [enable_monitoring=check])
+
# Check for ghc
AC_ARG_VAR(GHC, [ghc path])
AC_PATH_PROG(GHC, [ghc], [])
@@ -492,6 +499,33 @@ fi
AC_SUBST(ENABLE_CONFD, $has_confd)
AM_CONDITIONAL([ENABLE_CONFD], [test x$has_confd = xTrue])

+#extra modules for monitoring agent functionality
+has_monitoring=False
+if test "$enable_monitoring" != "no"; then
+ NO_ATTOPARSEC=-DNO_ATTOPARSEC
+ MONITORING_PKG=
+ AC_GHC_PKG_CHECK([attoparsec], [NO_ATTOPARSEC=], [MONITORING_PKG="$MONITORING_PKG attoparsec"])
+ if test -z "$MONITORING_PKG"; then
+ has_monitoring=True
+ else
+ if test "$enable_monitoring" = "check"; then
+ AC_MSG_WARN(m4_normalize([The required extra libraries for the monitoring
+ agent were not found ($MONITORING_PKG),
+ monitoring disabled]))
+ else
+ AC_MSG_FAILURE(m4_normalize([The monitoring functionality was requested, but
+ required libraries were not found:
+ $MONITORING_PKG]))
+ fi
+ fi
+fi
+if test "$has_monitoring" = "True"; then
+ AC_MSG_NOTICE([Enabling the monitoring agent usage])
+fi
+AC_SUBST(ENABLE_MONITORING, $has_monitoring)
+AC_SUBST(NO_ATTOPARSEC)
+AM_CONDITIONAL([ENABLE_MONITORING], [test x$has_monitoring = xTrue])
+
# development modules
HTOOLS_NODEV=
AC_GHC_PKG_CHECK([QuickCheck-2.*], [], [HTOOLS_NODEV=1], t)
diff --git a/lib/constants.py b/lib/constants.py
index 5bcccd9..218b191 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -139,6 +139,7 @@ ADOPTABLE_BLOCKDEV_ROOT = "/dev/disk/"
ENABLE_FILE_STORAGE = _autoconf.ENABLE_FILE_STORAGE
ENABLE_SHARED_FILE_STORAGE = _autoconf.ENABLE_SHARED_FILE_STORAGE
ENABLE_CONFD = _autoconf.ENABLE_CONFD
+ENABLE_MONITORING = _autoconf.ENABLE_MONITORING
ENABLE_SPLIT_QUERY = _autoconf.ENABLE_SPLIT_QUERY

NODED = "ganeti-noded"
--
1.7.7.3

[Michele Tartara]

Attoparsec is known to have had issues with parsing non-ASCII strings.
This test makes sure that parsing of Unicode characters works fine.

Also, added language extension CPP to test.hs, to allow it to load
the attoparsec tests only if the attoparsec library is actually
installed.

Signed-off-by: Michele Tartara <mtar...@google.com>
---

Makefile.am | 1 +

htest/Test/Ganeti/Attoparsec.hs | 72 +++++++++++++++++++++++++++++++++++++++
htest/test.hs | 7 ++++

3 files changed, 80 insertions(+), 0 deletions(-)
create mode 100644 htest/Test/Ganeti/Attoparsec.hs

diff --git a/Makefile.am b/Makefile.am
index 06e2e21..4e5031e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -473,6 +473,7 @@ HS_LIB_SRCS = \
htools/Ganeti/Utils.hs

HS_TEST_SRCS = \
+ htest/Test/Ganeti/Attoparsec.hs \
htest/Test/Ganeti/BasicTypes.hs \
htest/Test/Ganeti/Common.hs \
htest/Test/Ganeti/Confd/Utils.hs \
diff --git a/htest/Test/Ganeti/Attoparsec.hs b/htest/Test/Ganeti/Attoparsec.hs
new file mode 100644
index 0000000..fa6c576
--- /dev/null
+++ b/htest/Test/Ganeti/Attoparsec.hs
@@ -0,0 +1,72 @@

+{-# LANGUAGE TemplateHaskell #-}
+

+{-| Unittests for Attoparsec support for unicode -}

+
+{-
+
+Copyright (C) 2009, 2010, 2011, 2012 Google Inc.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301, USA.
+
+-}
+

+module Test.Ganeti.Attoparsec (testAttoparsec) where
+

+
+import Test.QuickCheck
+
+import Test.Ganeti.TestHelper
+import Test.Ganeti.TestCommon
+

+import qualified Data.Attoparsec.Text as A
+import Data.Attoparsec.Text (Parser)

+import Data.Text (pack, unpack)
+

+
+data SplitString = SplitString { name::String, value::String } deriving (Show)
+

+
+part1 :: String
+part1 = "äßĉ"
+
+part2 :: String
+part2 = "ðèق"
+

+
+{-| Simple parser able to split a string in two parts, name and value,
+ separated by a '=' sign -}

+simpleParser :: Parser SplitString
+simpleParser = do
+ n <- A.takeTill (\c -> A.isHorizontalSpace c || c == '=')
+ A.skipWhile A.isHorizontalSpace
+ _ <- A.char '='
+ A.skipWhile A.isHorizontalSpace
+ v <- A.takeTill A.isEndOfLine
+ return $ SplitString (unpack n) (unpack v)

+

+
+prop_parserSupportsUnicode :: Property
+prop_parserSupportsUnicode =
+ let (Right split) = A.parseOnly simpleParser (Data.Text.pack $ part1 ++
+ " = \t" ++ part2)
+ n = name split
+ v = value split
+ in (n ==? part1) .&&. (v ==? part2)
+
+

+testSuite "Attoparsec"

+ [ 'prop_parserSupportsUnicode
+ ]
diff --git a/htest/test.hs b/htest/test.hs

index 5180878..76df6fb 100644
--- a/htest/test.hs
+++ b/htest/test.hs
@@ -1,3 +1,4 @@
+{-# LANGUAGE CPP #-}
{-| Unittest runner for ganeti-htools.

-}
@@ -30,6 +31,9 @@ import Test.Framework
import System.Environment (getArgs)

import Test.Ganeti.TestImports ()
+#ifndef NO_ATTOPARSEC
+import Test.Ganeti.Attoparsec
+#endif
import Test.Ganeti.BasicTypes
import Test.Ganeti.Common
import Test.Ganeti.Confd.Utils
@@ -74,6 +78,9 @@ defOpts = TestOptions
allTests :: [Test]
allTests =
[ testBasicTypes
+#ifndef NO_ATTOPARSEC
+ , testAttoparsec
+#endif
, testCommon
, testConfd_Utils
, testDaemon
--
1.7.7.3

[Guido Trotter]

Note that everybody else does HTOOLS_LIBRARY, why do you do NO_ATTOPARSEC?
I would go for HTOOLS_ATTOPARSEC (in the future maybe we should get
rid of the htools name, but that's a separate issue).

Thanks,

Guido

[Michele Tartara]

I looked at HTOOLS_NOCURL: that's where the "NO" comes from.

Then I han no idea that the monitoring agent was to be considered part of the HTools. Is everything written in Haskell part of them?

In other words, should I go for: HTOOLS_NOATTOPARSEC or for something like MONITORING_NOATTOPARSEC?

Thanks,

Michele

[Guido Trotter]

Hi,

Yes, for now go for HTOOLS_NOATTOPARSEC as everything else that uses
haskell calls itself htools.
In the future we'll do some renaming there, but I'd rather do it "all
at the same time". :)

Thanks,

Guido

--
Guido Trotter
SRE - Corp Computing Services (aka Horsepower)
Google Germany

[Iustin Pop]

On Fri, Nov 09, 2012 at 10:31:53AM +0100, Guido Trotter wrote:
> Hi,
>
> Yes, for now go for HTOOLS_NOATTOPARSEC as everything else that uses
> haskell calls itself htools.
> In the future we'll do some renaming there, but I'd rather do it "all
> at the same time". :)

I think this is shortsighted/wrong.

Look at confd; there's no HTOOLS_CONFD or anything like that. I use
HTOOLS_NOCURL simply because it's a very old name, and I didn't bother
renaming it, but let's not add _more_ broken names.

I'll have other comments on the patch.

iustin

[Guido Trotter]

How about HLIB_ATTOPARSEC (or HLIB_NOATTOPARSEC) to clarify that it's
the attoparsec haskell library, then?
(NOATTOPARSEC by itself doesn't quite convey the meaning...)

Thanks,

Guido

[Iustin Pop]

On Fri, Nov 09, 2012 at 10:13:37AM +0100, Michele Tartara wrote:
> This will be needed for the data collectors of the monitoring agent.
>
> * Detection of the library
> * Creation of the appropriate variables
> * Update to the installation documentation

Thanks, this looks much better, I only have one question:

This looks like you are going to compile some components who can work
with either attoparsec or without.

Is this the case? I thought it's going to be like confd, where if we
don't have (e.g.) hinotify, we don't build the code at all. Only curl
has no_curl because htools can work with or without it; but I believe
monitoring will always depend on attoparsec.

thanks,
iustin

[Iustin Pop]

On Fri, Nov 09, 2012 at 10:48:46AM +0100, Guido Trotter wrote:
> How about HLIB_ATTOPARSEC (or HLIB_NOATTOPARSEC) to clarify that it's
> the attoparsec haskell library, then?
> (NOATTOPARSEC by itself doesn't quite convey the meaning...)

See my other reply, I think this is not needed at all, ha ha!

iustin

[Michele Tartara]

On Fri, Nov 9, 2012 at 10:50 AM, Iustin Pop <ius...@google.com> wrote:

This looks like you are going to compile some components who can work
with either attoparsec or without.

Is this the case? I thought it's going to be like confd, where if we
don't have (e.g.) hinotify, we don't build the code at all. Only curl
has no_curl because htools can work with or without it; but I believe
monitoring will always depend on attoparsec.

No, as you say, without attoparsec the whole monitoring cannot work. That's why the error message says "monitoring disabled", and the monitoring system will not be compiled at all.

Unfortunately, since the unit tests for attoparsec are part of the test.hs file, I'm using the -DNO_ATTOPARSEC in patch 2/2 through #ifndef to disable the compilation of the attoparsec unit test.

Unless there is a better way to do that, that I do not know due to my limited knowledge of Haskell... In that case, I'm more than willing to learn how to do that! :-)

 

Thanks,

Michele

[Iustin Pop]

I will teach you now a very secret and ultra-powerful trick! It's
called: changing the requirements :)

For development/unittests, we can absolutely require attoparsec. That's
fine. The only thing that we can't require is that building the
production code requires it.

So I think we can remove this, make the attoparsec tests always run and
require attoparsec, and simplify the build rules therefore :)

thanks,
iustin

[Michele Tartara]

Thanks for the "very secret and ultra-powerful trick".

Here is the new version of the patch.

---------------------------

diff --git a/configure.ac b/configure.ac

index 14554a3..f9a81c8 100644

--- a/configure.ac

+++ b/configure.ac

@@ -416,6 +416,13 @@ AC_ARG_ENABLE([confd],

   [],

   [enable_confd=check])

 

+ENABLE_MONITORING=

+AC_ARG_ENABLE([monitoring],

+  [AS_HELP_STRING([--enable-monitoring],

+  [enable the ganeti monitoring agent (default: check)])],

+  [],

+  [enable_monitoring=check])

+

 # Check for ghc

 AC_ARG_VAR(GHC, [ghc path])

 AC_PATH_PROG(GHC, [ghc], [])

@@ -492,12 +499,38 @@ fi

 AC_SUBST(ENABLE_CONFD, $has_confd)

 AM_CONDITIONAL([ENABLE_CONFD], [test x$has_confd = xTrue])

 

+#extra modules for monitoring agent functionality

+has_monitoring=False

+if test "$enable_monitoring" != "no"; then

+  MONITORING_PKG=

+  AC_GHC_PKG_CHECK([attoparsec], [], [MONITORING_PKG="$MONITORING_PKG attoparsec"])

+  if test -z "$MONITORING_PKG"; then

+    has_monitoring=True

+  else

+    if test "$enable_monitoring" = "check"; then

+      AC_MSG_WARN(m4_normalize([The required extra libraries for the monitoring

+                                agent were not found ($MONITORING_PKG),

+                                monitoring disabled]))

+    else

+      AC_MSG_FAILURE(m4_normalize([The monitoring functionality was requested, but

+                                   required libraries were not found:

+                                   $MONITORING_PKG]))

+    fi

+  fi

+fi

+if test "$has_monitoring" = "True"; then

+  AC_MSG_NOTICE([Enabling the monitoring agent usage])

+fi

+AC_SUBST(ENABLE_MONITORING, $has_monitoring)

+AM_CONDITIONAL([ENABLE_MONITORING], [test x$has_monitoring = xTrue])

+

 # development modules

 HTOOLS_NODEV=

 AC_GHC_PKG_CHECK([QuickCheck-2.*], [], [HTOOLS_NODEV=1], t)

 AC_GHC_PKG_CHECK([test-framework-0.6*], [], [HTOOLS_NODEV=1], t)

 AC_GHC_PKG_CHECK([test-framework-hunit], [], [HTOOLS_NODEV=1])

 AC_GHC_PKG_CHECK([test-framework-quickcheck2], [], [HTOOLS_NODEV=1])

+AC_GHC_PKG_CHECK([attoparsec], [], [HTOOLS_NODEV=1])

 if test -n "$HTOOLS_NODEV"; then

    AC_MSG_WARN(m4_normalize([Required development modules were not found,

                              you won't be able to run Haskell unittests]))

[Michele Tartara]

And here is the new version of this patch too, rewritten to work with the modifications applied to patch 1/2.

Thanks,

Michele

------------------------------------

diff --git a/Makefile.am b/Makefile.am

index d6bf864..9b769dd 100644

+part1 = "äß\u0109"

+

+part2 :: String

+part2 = "ðè\u0642"

index 5180878..b41fb99 100644

--- a/htest/test.hs

+++ b/htest/test.hs

@@ -30,6 +30,7 @@ import Test.Framework

 import System.Environment (getArgs)

 

 import Test.Ganeti.TestImports ()

+import Test.Ganeti.Attoparsec

 import Test.Ganeti.BasicTypes

 import Test.Ganeti.Common

 import Test.Ganeti.Confd.Utils

@@ -74,6 +75,7 @@ defOpts = TestOptions

 allTests :: [Test]

 allTests =

   [ testBasicTypes

+  , testAttoparsec

   , testCommon

   , testConfd_Utils

   , testDaemon

On Fri, Nov 9, 2012 at 10:13 AM, Michele Tartara <mtar...@google.com> wrote:

[Iustin Pop]

On Fri, Nov 09, 2012 at 02:54:54PM +0100, Michele Tartara wrote:
> Thanks for the "very secret and ultra-powerful trick".
> Here is the new version of the patch.
> ---------------------------

Thanks. This is wrapped by your client, so could you resend it via
git-send-email?

One more question:

> # development modules
> HTOOLS_NODEV=
> AC_GHC_PKG_CHECK([QuickCheck-2.*], [], [HTOOLS_NODEV=1], t)
> AC_GHC_PKG_CHECK([test-framework-0.6*], [], [HTOOLS_NODEV=1], t)
> AC_GHC_PKG_CHECK([test-framework-hunit], [], [HTOOLS_NODEV=1])
> AC_GHC_PKG_CHECK([test-framework-quickcheck2], [], [HTOOLS_NODEV=1])
> +AC_GHC_PKG_CHECK([attoparsec], [], [HTOOLS_NODEV=1])
> if test -n "$HTOOLS_NODEV"; then
> AC_MSG_WARN(m4_normalize([Required development modules were not found,
> you won't be able to run Haskell unittests]))

As you can see, before we didn't check any non-test libraries here. It's
good as it is for now, but can you add a FIXME saying that we should
unify checks for this? (e.g. also not run if hinotify not found, etc.
basically we would need to duplicate all tests, so this is not so good
and we should find a nicer way, later).

thanks,
iustin

[Michele Tartara]

This will be needed for the data collectors of the monitoring agent.

* Detection of the library
* Creation of the appropriate variables
* Update to the installation documentation

Signed-off-by: Michele Tartara <mtar...@google.com>
---

INSTALL | 13 ++++++++-----
configure.ac | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 5 deletions(-)

index 14554a3..3e134a2 100644

--- a/configure.ac
+++ b/configure.ac
@@ -416,6 +416,13 @@ AC_ARG_ENABLE([confd],
[],
[enable_confd=check])

+ENABLE_MONITORING=
+AC_ARG_ENABLE([monitoring],
+ [AS_HELP_STRING([--enable-monitoring],
+ [enable the ganeti monitoring agent (default: check)])],
+ [],
+ [enable_monitoring=check])
+
# Check for ghc
AC_ARG_VAR(GHC, [ghc path])
AC_PATH_PROG(GHC, [ghc], [])

@@ -492,12 +499,41 @@ fi

# development modules
HTOOLS_NODEV=
AC_GHC_PKG_CHECK([QuickCheck-2.*], [], [HTOOLS_NODEV=1], t)
AC_GHC_PKG_CHECK([test-framework-0.6*], [], [HTOOLS_NODEV=1], t)
AC_GHC_PKG_CHECK([test-framework-hunit], [], [HTOOLS_NODEV=1])
AC_GHC_PKG_CHECK([test-framework-quickcheck2], [], [HTOOLS_NODEV=1])

+# FIXME: unify checks for non-test libraries (attoparsec, hinotify, ...)
+# that are needed to execute the tests, avoiding the duplication
+# of the checks.

+AC_GHC_PKG_CHECK([attoparsec], [], [HTOOLS_NODEV=1])
if test -n "$HTOOLS_NODEV"; then
AC_MSG_WARN(m4_normalize([Required development modules were not found,
you won't be able to run Haskell unittests]))

--
1.7.7.3

[Michele Tartara]

Attoparsec is known to have had issues with parsing non-ASCII strings.
This test makes sure that parsing of Unicode characters works fine.

Also, added language extension CPP to test.hs, to allow it to load
the attoparsec tests only if the attoparsec library is actually
installed.

Signed-off-by: Michele Tartara <mtar...@google.com>
---
Makefile.am | 1 +
htest/Test/Ganeti/Attoparsec.hs | 72 +++++++++++++++++++++++++++++++++++++++

htest/test.hs | 2 +
3 files changed, 75 insertions(+), 0 deletions(-)

create mode 100644 htest/Test/Ganeti/Attoparsec.hs

diff --git a/Makefile.am b/Makefile.am

index 5180878..b41fb99 100644
--- a/htest/test.hs
+++ b/htest/test.hs
@@ -30,6 +30,7 @@ import Test.Framework
import System.Environment (getArgs)

import Test.Ganeti.TestImports ()
+import Test.Ganeti.Attoparsec
import Test.Ganeti.BasicTypes
import Test.Ganeti.Common
import Test.Ganeti.Confd.Utils
@@ -74,6 +75,7 @@ defOpts = TestOptions
allTests :: [Test]
allTests =
[ testBasicTypes
+ , testAttoparsec
, testCommon
, testConfd_Utils
, testDaemon

--
1.7.7.3

[Iustin Pop]

On Fri, Nov 09, 2012 at 04:48:21PM +0100, Michele Tartara wrote:
> Attoparsec is known to have had issues with parsing non-ASCII strings.
> This test makes sure that parsing of Unicode characters works fine.
>
> Also, added language extension CPP to test.hs, to allow it to load
> the attoparsec tests only if the attoparsec library is actually
> installed.

Is this comment still true/needed?

The code itself is LGTM, thanks.

iustin

[Iustin Pop]

On Fri, Nov 09, 2012 at 04:46:05PM +0100, Michele Tartara wrote:
> This will be needed for the data collectors of the monitoring agent.
>
> * Detection of the library
> * Creation of the appropriate variables
> * Update to the installation documentation
>
> Signed-off-by: Michele Tartara <mtar...@google.com>

LGTM, thanks.

iustin

[Michele Tartara]

List added back

On Tue, Nov 13, 2012 at 10:46 AM, Michele Tartara <mtar...@google.com> wrote:

Oh, no, as you can see from the patch itself, CPP is not used anymore.

Should I send a fixed patch with the modified comment, or can you fix it while pushing it?

Thanks, 

Michele

[Iustin Pop]

On Tue, Nov 13, 2012 at 10:46:35AM +0100, Michele Tartara wrote:
> List added back
>
>
> On Tue, Nov 13, 2012 at 10:46 AM, Michele Tartara <mtar...@google.com>wrote:
>
> > Oh, no, as you can see from the patch itself, CPP is not used anymore.
> >
> > Should I send a fixed patch with the modified comment, or can you fix it
> > while pushing it?

I'll fix while pushing. Just asking for confirmation :)

LGTM & thanks,
iustin

[Iustin Pop]

Actually I was too careless, there are a few code style issues, and one
actual code bug in your test code.

Style issues:

- trailing whitespace; please configure your editor appropriately
- missing docstrings; we do want docstrings even on constants (e.g.
"Unicode test string, first part.", "Unicode test string, second
part.")

And to the code bug: never, ever "let (Right foo) = …". This will lead
to a runtime error, which is a big no-no. You should always deconstruct
via case:

case A.parseOnly simpleParser … of
Right y -> …
Left msg -> failTest $ "Failed to parse: " ++ msg

As a side-comment, defining a separate type (SplitString) just for one
use seems over doing it. I personally would just return a tuple (String,
String) from simpleParser…

thanks,
iustin

[Michele Tartara]

Ok, I'll fix this immediately.

Thanks,

Michele

[Iustin Pop]

On Tue, Nov 13, 2012 at 11:00:55AM +0100, Iustin Pop wrote:
> On Tue, Nov 13, 2012 at 10:52:55AM +0100, Iustin Pop wrote:
> > On Tue, Nov 13, 2012 at 10:46:35AM +0100, Michele Tartara wrote:
> > > List added back
> > >
> > >
> > > On Tue, Nov 13, 2012 at 10:46 AM, Michele Tartara <mtar...@google.com>wrote:
> > >
> > > > Oh, no, as you can see from the patch itself, CPP is not used anymore.
> > > >
> > > > Should I send a fixed patch with the modified comment, or can you fix it
> > > > while pushing it?
> >
> > I'll fix while pushing. Just asking for confirmation :)
> >
> > LGTM & thanks,
>
> Actually I was too careless, there are a few code style issues, and one
> actual code bug in your test code.
>
> Style issues:
>
> - trailing whitespace; please configure your editor appropriately
> - missing docstrings; we do want docstrings even on constants (e.g.
> "Unicode test string, first part.", "Unicode test string, second
> part.")
>
> And to the code bug: never, ever "let (Right foo) = …". This will lead
> to a runtime error, which is a big no-no. You should always deconstruct
> via case:
>
> case A.parseOnly simpleParser … of
> Right y -> …
> Left msg -> failTest $ "Failed to parse: " ++ msg

FYI, this would be detected and enforced by the compiler, but we're
still supporting ghc 6.12, whereas this flag appeared only in 7.2. Me
wants newer compilers!!

Hmmm, I wonder if I can't change the build system to detect this and
enable this (not-automatically enable-able via -Wall) flag…

thanks,
iustin

[Michele Tartara]

Attoparsec is known to have had issues with parsing non-ASCII strings.
This test makes sure that parsing of Unicode characters works fine.

Signed-off-by: Michele Tartara <mtar...@google.com>
---
Makefile.am | 1 +

htest/Test/Ganeti/Attoparsec.hs | 69 +++++++++++++++++++++++++++++++++++++++
htest/test.hs | 2 +
3 files changed, 72 insertions(+), 0 deletions(-)

create mode 100644 htest/Test/Ganeti/Attoparsec.hs

diff --git a/Makefile.am b/Makefile.am

index ea38407..d6a0e28 100644

--- a/Makefile.am
+++ b/Makefile.am
@@ -473,6 +473,7 @@ HS_LIB_SRCS = \
htools/Ganeti/Utils.hs

HS_TEST_SRCS = \
+ htest/Test/Ganeti/Attoparsec.hs \
htest/Test/Ganeti/BasicTypes.hs \
htest/Test/Ganeti/Common.hs \
htest/Test/Ganeti/Confd/Utils.hs \
diff --git a/htest/Test/Ganeti/Attoparsec.hs b/htest/Test/Ganeti/Attoparsec.hs
new file mode 100644

index 0000000..14ddd9c
--- /dev/null
+++ b/htest/Test/Ganeti/Attoparsec.hs
@@ -0,0 +1,69 @@

+{-| Unicode test string, first part -}

+part1 :: String
+part1 = "äßĉ"
+

+{-| Unicode test string, second part -}

+part2 :: String
+part2 = "ðèق"
+
+
+{-| Simple parser able to split a string in two parts, name and value,
+ separated by a '=' sign -}

+simpleParser :: Parser (String, String)

+simpleParser = do
+ n <- A.takeTill (\c -> A.isHorizontalSpace c || c == '=')
+ A.skipWhile A.isHorizontalSpace
+ _ <- A.char '='
+ A.skipWhile A.isHorizontalSpace
+ v <- A.takeTill A.isEndOfLine

+ return (unpack n, unpack v)
+
+
+-- | Tests whether a Unicode string is still Unicode after being parsed
+prop_parserSupportsUnicode :: Property
+prop_parserSupportsUnicode = case A.parseOnly simpleParser text of
+ Right (name, value) -> (name ==? part1) .&&. (value ==? part2)
+ Left msg -> failTest $ "Failed to parse: " ++ msg
+ where text = Data.Text.pack $ part1 ++ " = \t" ++ part2

[Iustin Pop]

On Tue, Nov 13, 2012 at 12:31:13PM +0100, Michele Tartara wrote:
> Attoparsec is known to have had issues with parsing non-ASCII strings.
> This test makes sure that parsing of Unicode characters works fine.

LGTM, thanks. I'll push with just some trivial docstring/style changes.

iustin

[Michele Tartara]

Ok, thanks, Next time I'll use the right docstring format.

Michele