Re: hackage importer

[ludo@gnu.org (Ludovic Courtès)]

Federico Beffa <beffa@ieee.org> skribis:

> On Sun, Mar 29, 2015 at 3:58 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>> On Thu, Mar 26, 2015 at 2:09 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>> Could you post the actual backtrace you get (?) when running the program
>>>> with LC_ALL=C?
>>>
>>> I doesn't backtrace, the function just gives the wrong result.
>>
>> Hmm, OK.  Still sounds like an encoding error.
>
> After changing the character that I mentioned in the previous email it
> works correctly with LC_ALL=C.

Well, OK.  We may still be doing something wrong wrt. encoding/decoding,
but I’m not sure what.

>>> Before working further on improving the interface, I want first to
>>> understand what are the root causes of the errors (especially the one
>>> causing the backtrace) and fix them.
>
> The problems turned out to be related to:
> * the use of TABs in some .cabal files. I've now updated a couple of regexp.
> * The following odd indentation which confused the parsing (this is
> the one which caused the backtrace):
>
>   build-depends:
>       base >= 4.3 && < 4.9
>     , bytestring
>     , filepath
> ...
>
> I've now improved the algorithm which can now handle this odd indentation.

Nice.  TABs and odd indentation probably make good additional test cases
to have in tests/cabal.scm.

> I've now tested the importer with ca. 40 packages and (I believe) they
> are all handled without errors.

Woohoo!

>> Would it be possible for ‘read-cabal’ to instead return a
>> tree-structured sexp like:
>>
>>   (if (os windows)
>>       (build-depends (Win32 >= 2 && < 3))
>>       (build-depends (unix >= 2.0 && < 2.8)))
>>
>> That would use a variant of ‘conditional->sexp-like’, essentially.
>>
>> (Of course the achieve that the parser must keep track of indentation
>> levels and everything, as you already implemented; I’m just commenting
>> on the interface here.)
>>
>> Then, if I could imagine:
>>
>>   (eval-cabal '(("name" "foo")
>>                 ("version" "1.0"
>>                 ("executable cabal" (if (os windows) ...)))
>>   => #<cabal-package name: "foo" dependencies: '(unix)>
>>
>> This way the structure of the Cabal file would be preserved, only
>> converted to sexp form, which is easier to work with.
>>
>> Does that make sense?
>
> To be honest, I'm not sure I understand what you would like to achieve.

It’s really just about the architecture and layers of code.

> 'read-cabal' returns an object and, according to your proposal, you
> would like a function '(eval-cabal object)' returning a package. In
> the code that is exactly what '(hackage-module->sexp object)' does. Is
> it a matter of naming? (I've taken names from the python and perl
> build systems, but of course I can change them if desired.)

I think it’s a matter of separating concerns.  In my mind there are
three distinct layers:

  1. Cabal parsing (what I call ‘read-cabal’, because it’s the
     equivalent of ‘read’);

  2. Cabal evaluation/instantiation for a certain set of flags, OS,
     etc. (what I call ‘eval-cabal’ because it’s the equivalent of
     ‘eval’);

  3. Conversion of Cabal packages of Guix package sexps.

My concern was about making sure these three phases were clearly visible
in the code.  Tu put it differently, #1 and #2 would conceptually be
part of a Cabal parsing/evaluation library, while #3 would be the only
Guix-specific part.

> Right now 'read-cabal' is fairly simple and easy to read and debug.
> Some complexity for the evaluation of conditionals is postponed and
> handled by the function '(dependencies-cond->sexp object)' which is
> used internally by '(hackage-module->sexp object)' to create the
> package.
>
> As far as I understand, you would like 'read-cabal' to directly
> evaluate conditionals.

No, precisely not.  I’m saying ‘read-cabal’ should include an AST of
conditionals; that AST would be evaluated by ‘eval-cabal’.

Anyway, I’ve probably used enough of your time by now.  :-)
If this discussion gives you ideas on how to structure the code, that is
fine, but otherwise we can probably go with the architecture you
propose.

How does that sound?

Thanks,
Ludo’.