Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test fail in lwt_unix #729

Closed
olafhering opened this issue Oct 4, 2019 · 16 comments · Fixed by #732
Closed

test fail in lwt_unix #729

olafhering opened this issue Oct 4, 2019 · 16 comments · Fixed by #732
Labels
Milestone

Comments

@olafhering
Copy link
Contributor

A bytecode_only ocaml-4.05 fails in runcheck:

[   81s] $ (cd _build/default/test/unix && ./main.exe)
[   81s] Testing library 'unix'...
[   81s] ..............SSS.....................................................................................................SS...FFFFF..................................................SS................
[   81s] Test 'openfile: O_CLOEXEC' in suite 'lwt_unix' produced 'false'
[   81s] Test 'openfile: O_CLOEXEC not given' in suite 'lwt_unix' produced 'false'
[   81s] Test 'openfile: O_KEEPEXEC' in suite 'lwt_unix' produced 'false'
[   81s] Test 'openfile: O_CLOEXEC, O_KEEPEXEC' in suite 'lwt_unix' produced 'false'
[   81s] Test 'openfile: O_KEEPEXEC, O_CLOEXEC' in suite 'lwt_unix' produced 'false'
[   81s] error: Bad exit status from /var/tmp/rpm-tmp.8N96N3 (%check)
[   81s] 
[   81s] 
[   81s] RPM build errors:
[   81s]     Bad exit status from /var/tmp/rpm-tmp.8N96N3 (%check)
@olafhering
Copy link
Contributor Author

@aantron aantron added this to the 4.3.2 milestone Oct 4, 2019
@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

Lwt doesn't install on 4.05.0+bytecode-only because its dependency seq assumes a native-code compiler is installed, so I will fix that first. Are you using a patched seq in your build? If so, where can I find the patch? If not, how is the bytecode-only variant configured for you?

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

c-cube/seq#5

@aantron aantron changed the title test fail in ltw_unix test fail in lwt_unix Oct 4, 2019
@olafhering
Copy link
Contributor Author

There is nothing special in seq, it is used as is: devel:languages:ocaml:bytecode_only/ocaml-seq

@olafhering
Copy link
Contributor Author

c-cube/seq#5

This assumes opam is used. But opam is not required to use ocaml.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

Yes, most of us use multiple package managers with OCaml, you may note the text allows for that :) I linked the issue for me to keep track of it, not because I expect it to affect your builds. I needed that patch to reproduce the problem here, which I'm now debugging. Other opam users will benefit from it.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

I am concerned however, about why seq 0.1 is working in your environment. This suggests either a configuration of OCaml that built ocamlopt anyway, or a stray ocamlopt in your environment. It's also possible that you have a different or patched Ocamlbuild, or there may be some other explanation for why the .cmxa and .cmxs targets appear to be built successfully.

@olafhering
Copy link
Contributor Author

Looking through my ocaml.spec, there is just a make world, instead of a make world.opt. I think this is all it takes, and dune can deal with the result.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

Ok, the explanation for the difference with seq in opam is that a port to Dune was done in this fork and not upstreamed to seq, and not released to opam. build.opensuse.org is using this fork. That's for anyone else using opam that hits this issue in the near future. c-cube/seq#6 (comment), c-cube/seq#7

@olafhering
Copy link
Contributor Author

Ah, yes. There are quite some repositories without dune support, and that project fixes many of them.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

I guess the only relevant result of the seq business for openSUSE, is that seq is about to be released again at version 0.2 (I suggest to watch ocaml/opam-repository#14998). The 0.1 released (or proposed?) for openSUSE right now is in fact not 0.1, it's a patched version that should have been 0.2, and the 0.1 name is misleading. None of this particularly matters, since the code is the same, but for the sake of tidiness, I weakly suggest to wait for the impending seq release, and start seq off at 0.2, and point the URL to the main repo :)

I've proceeded with the actual issue locally in the meantime :)

EDIT: dune-universe/seq#1 about deleting that repo now.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

@olafhering, I'm also remembering you mentioning about a (false) dependency cycle: lwt -> bisect_ppx -> ounit. This would only happen because ounit is a monorepo that also includes another package, ounit-lwt (see the .opam files in the root directory of ounit).

That makes me concerned: which command is openSUSE using to build ounit (and perhaps also other packages)? For example, doing dune build typically builds all packages in a monorepo, but doing dune build -p ounit will build only the package ounit

The latter is the standard command to issue in the OCaml ecosystem, for non-development builds, because many repos function as monorepos for a main package, and some secondary helper packages.

In the same way, I see that the openSUSE Lwt package includes react as a dependency. But, unless you are intentionally packaging Lwt_react, it shouldn't be there.

You may be intentionally packaging ounit with all its helpers, and Lwt with all its helpers, but that will lead to occasional dependency cycles, as above, because it is not an accurate translation of the true OCaml package boundaries.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

...and Bisect_ppx is not a dependency of Lwt releases at all. It's a dev dependency that we have to manually patch out until ocaml/dune#57 is fixed.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

Even more, ounit is a dev-only dependency of Bisect_ppx!

If your build command is dune build, Dune will build all packages in the repo with a development profile, which often pulls in tests, and therefore dev dependencies.

If your command is dune build -p package-name, Dune will build only the given package, and the minimum needed, usually not including tests, and therefore not pulling in dev dependencies.

I'm not sure how to interpret the openSUSE package metadata, but you basically want all these deps in your testing environment. You shouldn't include them for releases.

@olafhering
Copy link
Contributor Author

olafhering commented Oct 4, 2019

Thanks for your comments. I will dig into the actual differences between the 'dev' and 'release' targets of dune.

Typically a copy of a given git snapshot/tag is packaged, then dune build @install runs, and also dune runtest [ || : ignore failures ]. Right now that appears to work well, modulo the spurious failures I reported.

I already thought about how to untangle cycles, like ounit may depend on something else, which in turn may use ounit to test itself.

It would be cool if the dune runtest could use system libraries, instead of the one that was just built. That way the step to create a ocaml-foo.rpm from ocaml-foo.spec could be separated from a ocaml-foo-test.spec, which would just run the testsuite of ocaml-foo. This comes with the risk that a bug in ocaml-foo would be silently propagated down the dependency chain, it would only show up in ocaml-foo-test. Right now a failure in ocaml-foo would stop the chain right there.

@aantron
Copy link
Collaborator

aantron commented Oct 4, 2019

Great, I'll just add a few more comments for whenever you're looking into it.

  • These two sections of the Dune manual show the options for building release packages: 1 2. Even though the second one says it's for opam, it's really for all package managers, the Dune docs just more or less assume opam as "the" package manager.

  • If your metadata is careful about both dev packages and not building everything in a monorepo, you will have no dependency cycles (except for the few still lurking in the more obscure corners of the OCaml ecosystem). It will, of course, also greatly reduce the amount of OCaml packages you have to deal with in openSUSE.

  • IIRC, you are packaging Lwt because it was seemingly needed for ounit. But, if you truly only need ounit and not ounit-lwt, then, when you no longer try to build ounit-lwt, you won't have to package Lwt at all :) I'll still fix the underlying problem in this issue, however (and any other issues you may find, now or later).

  • Probably the safest way to check if a git repo can be used directly, is to download the opam archive for a release, and diff it against the corresponding git tag (they can differ due to package submission tools). You should also look at the commit under the git tag, as it might be on a leaf branch whose purpose is specifically to delete dev code right before release. If all these diffs are only to metadata (mainly the .opam file), then using git directly is safe. This will be the case for the vast majority of repos. For a few repos, like Lwt, there is a small adjustment between the git version and the release (the patch I linked earlier). In general, though, we are working to get rid of any need for such adjustments, so even more git versions will be usable directly as releases. You can find links to the release archives on opam.ocaml.org. For example, at the bottom of this page, is the archive for Lwt 4.3.1. If you need programmatic access, probably the easiest way is to clone opam-repository.

  • It would be cool if the dune runtest could use system libraries, instead of the one that was just built

    Wouldn't you want runtest to run against the copy of the library that was just built, with all other libraries it depends on taken from the system installation? I think this is what a lot of the code that is written by package authors for runtest assumes, too. Since that seems to be your current scheme, and it matches what the OCaml ecosystem expects, I'd suggest to keep it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants