[bug#59487,v2,1/2] build-system/dune: Automatically deduce test-target in most cases.

From: raingloom <raingloom@riseup.net>

guix/build-system/dune.scm (dune-build): tests? defaults to #f.
guix/build/dune-build-system.scm (check): Missing test-target is auto-detected.
---
 guix/build-system/dune.scm       |  2 +-
 guix/build/dune-build-system.scm | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)
  

Thanks for this! I think it is a valuable improvement.

Csepp <raingloom@riseup.net> writes:

> From: raingloom <raingloom@riseup.net>
>
> guix/build-system/dune.scm (dune-build): tests? defaults to #f.

This should be: "test-target defaults to #f".

> +    (let ((program (if jbuild? "jbuilder" "dune"))
> +          (test-target (or test-target
> +                           (cond
> +                            ((file-exists? "tests") "tests")
> +                            ((file-exists? "test") "test")
> +                            (else ".")))))
> +      (apply invoke program "runtest"
> +             (append (if test-target (list test-target) '())
> +                     (if package (list "-p" package)
>                           dune-release-flags)
>                       test-flags))))
>    #t)

I think what Julien ment (and I agree) is that you can completely drop
the checks for the files/directories "tests" or "test" to exist.

In your patch, if test-target is #f and "test" or "tests" do not exist,
then the we will run:

`dune runtest -p package .`

but we could (and maybe should) run just:

`dune runtest -p package`

In fact, we should run this even if the "test" or "tests" directories
exist (otherwise we might miss running some tests placed in other
directories).

So this would be enough:

> +    (let ((program (if jbuild? "jbuilder" "dune")))
> +      (apply invoke program "runtest"
> +             (append (if test-target (list test-target) '())
> +                     (if package (list "-p" package)
>                           dune-release-flags)
>                       test-flags)))

Thinking of this, maybe we can drop the `test-target` argument
completely. wdyt?

-pukkamustard
  

pukkamustard <pukkamustard@posteo.net> writes:

> Thinking of this, maybe we can drop the `test-target` argument
> completely. wdyt?

Yeah, I'm pretty sure we should drop the test-target argument. I don't
see any reason why it should be used with the dune build-system.

The two remaining packages that have a test-target that is not "test",
"tests" or "." (and do not have tests disabled):

- ocaml-frontc: Builds as expected when test-target is removed.
- ocaml-cohttp: test-target points to a test helper library, not the
  tests themselves. Removing the test-target runs the tests, which
  currently fail.

-pukkamustard
  

pukkamustard <pukkamustard@posteo.net> writes:

> Thanks for this! I think it is a valuable improvement.
>
> Csepp <raingloom@riseup.net> writes:
>
>> From: raingloom <raingloom@riseup.net>
>>
>> guix/build-system/dune.scm (dune-build): tests? defaults to #f.
>
> This should be: "test-target defaults to #f".
>
>> +    (let ((program (if jbuild? "jbuilder" "dune"))
>> +          (test-target (or test-target
>> +                           (cond
>> +                            ((file-exists? "tests") "tests")
>> +                            ((file-exists? "test") "test")
>> +                            (else ".")))))
>> +      (apply invoke program "runtest"
>> +             (append (if test-target (list test-target) '())
>> +                     (if package (list "-p" package)
>>                           dune-release-flags)
>>                       test-flags))))
>>    #t)
>
> I think what Julien ment (and I agree) is that you can completely drop
> the checks for the files/directories "tests" or "test" to exist.
>
> In your patch, if test-target is #f and "test" or "tests" do not exist,
> then the we will run:
>
> `dune runtest -p package .`
>
> but we could (and maybe should) run just:
>
> `dune runtest -p package`
>
> In fact, we should run this even if the "test" or "tests" directories
> exist (otherwise we might miss running some tests placed in other
> directories).
>
> So this would be enough:
>
>> +    (let ((program (if jbuild? "jbuilder" "dune")))
>> +      (apply invoke program "runtest"
>> +             (append (if test-target (list test-target) '())
>> +                     (if package (list "-p" package)
>>                           dune-release-flags)
>>                       test-flags)))
>
> Thinking of this, maybe we can drop the `test-target` argument
> completely. wdyt?
>
> -pukkamustard

Hmm, makes sense.  I'll test what complete removal does.
  

Csepp <raingloom@riseup.net> writes:

> pukkamustard <pukkamustard@posteo.net> writes:
>
>> Thanks for this! I think it is a valuable improvement.
>>
>> Csepp <raingloom@riseup.net> writes:
>>
>>> From: raingloom <raingloom@riseup.net>
>>>
>>> guix/build-system/dune.scm (dune-build): tests? defaults to #f.
>>
>> This should be: "test-target defaults to #f".
>>
>>> +    (let ((program (if jbuild? "jbuilder" "dune"))
>>> +          (test-target (or test-target
>>> +                           (cond
>>> +                            ((file-exists? "tests") "tests")
>>> +                            ((file-exists? "test") "test")
>>> +                            (else ".")))))
>>> +      (apply invoke program "runtest"
>>> +             (append (if test-target (list test-target) '())
>>> +                     (if package (list "-p" package)
>>>                           dune-release-flags)
>>>                       test-flags))))
>>>    #t)
>>
>> I think what Julien ment (and I agree) is that you can completely drop
>> the checks for the files/directories "tests" or "test" to exist.
>>
>> In your patch, if test-target is #f and "test" or "tests" do not exist,
>> then the we will run:
>>
>> `dune runtest -p package .`
>>
>> but we could (and maybe should) run just:
>>
>> `dune runtest -p package`
>>
>> In fact, we should run this even if the "test" or "tests" directories
>> exist (otherwise we might miss running some tests placed in other
>> directories).
>>
>> So this would be enough:
>>
>>> +    (let ((program (if jbuild? "jbuilder" "dune")))
>>> +      (apply invoke program "runtest"
>>> +             (append (if test-target (list test-target) '())
>>> +                     (if package (list "-p" package)
>>>                           dune-release-flags)
>>>                       test-flags)))
>>
>> Thinking of this, maybe we can drop the `test-target` argument
>> completely. wdyt?
>>
>> -pukkamustard
>
> Hmm, makes sense.  I'll test what complete removal does.

Well ocaml-ppxlib fails and I should be working my MirageOS thesis
stuff, so I'll leave this up to you.  Or I'll get back to it once I
figure out how cross-toolchains are supposed to work.