Re: [PATCH]: Add IcedTea 6

[Ludovic Courtès]

Ricardo Wurmus <address@hidden> skribis:

> Ludovic Courtès writes:

[...]

>>> +           (substitute* "Makefile.in"
>>> +             ;; link against libgcj to avoid linker error
>>> +             (("-o native-ecj")
>>> +              "-lgcj -o native-ecj")
>>> +             ;; do not leak information about the build host
>>> +             (("DISTRIBUTION_ID=\"\\$\\(DIST_ID\\)\"")
>>> +              "DISTRIBUTION_ID=\"\\\"guix\\\"\""))
>>
>> This could go to ‘snippet’ because it’s independent of the inputs used
>> for the build, but do whatever seems most appropriate to you in terms of
>> structure.
>
> I made this a snippet.  Other things that would be good as snippets
> (such as the patch to remove the timestamps) can only be done after the
> build has started, because IcedTea patches the unpacked sources quite a
> bit.

OK.

[...]

> I tried this but ran into a couple of problems.  Not all instances of
> /bin are bad.  There are many places in the tests where /bin follows a
> variable (and these are totally fine).  I also don't want to patch
> shebangs twice.  The character class would have to be picked carefully
> as well, because commands are sometimes quoted or inside braces etc.
>
> Here's what I used before I gave up ([^[:space:]] instead of
> [[:graphic:]] because the latter does not seem to exist and because it
> would catch all sorts of unwanted characters if it did):
>
>  (substitute* (find-files "." "\\.(sh|java)$")
>    (("[^!](/usr)?/bin/([^[:space:]]+)" _ _ command)
>     (or (which command) command)))

OK.  Well, let’s keep it the way you initially wrote it.

>>> +            (alist-replace
>>> +             'check
>>> +             (lambda _
>>> +               (and (zero? (system* "make" "check-hotspot"))
>>> +                    (zero? (system* "make" "check-langtools"))
>>> +                    (zero? (system* "make" "check-jdk"))))
>>
>> Maybe add a comment that the check- targets always return zero?
>
> I rewrote this to instead check the generated test logs in test/ for
> lines starting with "Error:" or "FAILED:".  To disable tests I wrapped
> it in an (or #t ...) as the argument #:tests? #f does not disable a
> custom check phase.
>
>              'check
>              (lambda _
>                ;; The "make check-*" targets always return zero, so we need to
>                ;; check for errors in the associated log files to determine
>                ;; whether any tests have failed.
>                (use-modules (ice-9 rdelim))
>                (let* ((error-pattern (make-regexp "^(Error|FAILED):.*"))
>                       (checker (lambda (port)
>                                  (let loop ((_ #t))

You can make it (let loop () ... (loop)).

>                                   (let ((line (read-line port)))
>                                     (cond
>                                      ((eof-object? line) #t)
>                                      ((regexp-exec error-pattern line) #f)
>                                      (else (loop #t)))))))
>                       (run-test (lambda (test)
>                                   (system* "make" test)
>                                   (call-with-input-file
>                                       (string-append "test/" test ".log")
>                                     checker))))
>                  (or #t ; skip tests
>                      (and (run-test "check-hotspot")
>                           (run-test "check-langtools")
>                           (run-test "check-jdk")))))

OK.

>>> +    (description
>>> +     "The IcedTea project provides a harness to build the source code from
>>> +http://openjdk.java.net using Free Software build tools along with 
>>> additional
>>> +features such as a Free Software plugin and web start implementation and an
>>> +LLVM-based JIT.")
>>
>> No good idea here.  :-)
>
> How about this:
>
>    "The OpenJDK built with the IcedTea build harness."
>
> It doesn't sound great but it actually says what it is.

Fine with me.

One last thing: perhaps we can directly remove mips64el-linux from
‘supported-platforms’?

OK to push.  

Thanks,
Ludo’.