Skip to content

[new release] tcpip (6.1.0)#18357

Closed
dinosaure wants to merge 2 commits into
ocaml:masterfrom
dinosaure:release-tcpip-6.1.0
Closed

[new release] tcpip (6.1.0)#18357
dinosaure wants to merge 2 commits into
ocaml:masterfrom
dinosaure:release-tcpip-6.1.0

Conversation

@dinosaure
Copy link
Copy Markdown
Contributor

OCaml TCP/IP networking stack, used in MirageOS

CHANGES:

@camelus
Copy link
Copy Markdown
Contributor

camelus commented Mar 17, 2021

Commit: dbfddf8

A pull request by opam-seasoned @dinosaure.

☀️ All lint checks passed dbfddf8
  • These packages passed lint tests: tcpip.6.1.0

☀️ Installability check (+1)
  • new installable packages (1): tcpip.6.1.0

@mseri
Copy link
Copy Markdown
Member

mseri commented Mar 18, 2021

A test failed on the latest Ubuntu with

# ┌──────────────────────────────────────────────────────────────────────────────┐
# │ [FAIL]        socket                0   two sockets connect via TCP.         │
# └──────────────────────────────────────────────────────────────────────────────┘
# test.exe: [INFO] IPv4 socket stack: connect
# test.exe: [INFO] IPv4 socket stack: connect
# [exception] Unix.Unix_error(Unix.EADDRINUSE, "bind", "")
#             Raised by primitive operation at Tcpip_stack_socket.V4.listen_tcpv4 in file "src/stack-unix/tcpip_stack_socket.ml", line 70, characters 6-88
#             Called from Dune__exe__Test_socket.two_connect_tcp.(fun) in file "test/test_socket.ml", line 40, characters 2-60
#             Called from Dune__exe__Test.run in file "test/test.ml" (inlined), line 37, characters 15-24
#             Called from Dune__exe__Test.(fun) in file "test/test.ml", line 51, characters 42-47
#             Called from Alcotest_engine__Core.Make.protect_test.(fun) in file "src/alcotest-engine/core.ml", line 277, characters 17-23
#             Called from Alcotest_engine__Monad.Identity.catch in file "src/alcotest-engine/monad.ml", line 34, characters 31-35
#             

Is this a known Heisenbug of the tests or needs to be investigated?

@dinosaure
Copy link
Copy Markdown
Contributor Author

I think it's related to mirage/mirage-tcpip#437, let me look into it and try to find a solution.

CHANGES:

* checksum stubs: Drop `caml_` from their name (@hannesm, mirage/mirage-tcpip#445)
* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
@dinosaure dinosaure force-pushed the release-tcpip-6.1.0 branch from 661b80c to dbbe1e2 Compare March 26, 2021 10:44
@dinosaure
Copy link
Copy Markdown
Contributor Author

I re-released mirage-tcpip with a fix about cancelation (thx @hannesm & @talex5), this release should be fine now.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Mar 26, 2021

please hold on, on Wednesday we discussed to avoid the caml_ prefix in the C stub names -- would you mind to take a look at mirage/mirage-tcpip#445 and include this in the release?

@dinosaure dinosaure force-pushed the release-tcpip-6.1.0 branch from dbbe1e2 to 8dc4803 Compare March 26, 2021 12:50
@dinosaure
Copy link
Copy Markdown
Contributor Author

Release was redone including the patch of @hannesm 👍.

@mseri
Copy link
Copy Markdown
Member

mseri commented Mar 26, 2021

Should we worry about those three EADDRINUSE failures? (Seems they have not yet gone)

@dinosaure
Copy link
Copy Markdown
Contributor Author

Should we worry about those three EADDRINUSE failures? (Seems they have not yet gone)

Hmmmhmm, it's really strange that in our CI, this bug does not appear and it appears on the opam-ci. Then, we delayed the release in order to fix this bug... I don't really know what happens and don't have enough time to continue to deeply look on it 😕 (/cc @mirage/core)

@sternenseemann
Copy link
Copy Markdown

On NixOS never hit these failures (to the point where I accidentally updated tcpip to 6.1.0 at the first github release because I forgot to check if the opam PR was merged already 😬).

@mseri
Copy link
Copy Markdown
Member

mseri commented Mar 26, 2021

I don’t mind merging, you just have to give me the thumbs up for doing it

@dinosaure
Copy link
Copy Markdown
Contributor Author

For my point of view, it's not acceptable to release mirage-tcpip as is 😕 . I hope that someone more confident with the code understand such error and I believe that it will take a long time. So, it's better to close I think.

@dinosaure dinosaure closed this Mar 26, 2021
@mseri
Copy link
Copy Markdown
Member

mseri commented Mar 26, 2021

Do you know if it has ever happened before? (The error I mean)

@dinosaure
Copy link
Copy Markdown
Contributor Author

As far as I can tell, no but it seems serious when opam-ci reproduced it several times.

@kit-ty-kate kit-ty-kate reopened this Mar 26, 2021
@kit-ty-kate
Copy link
Copy Markdown
Member

Do you know if it has ever happened before? (The error I mean)

This sort of failure is rather common with mirage software (I'm looking into the past logs to get a list of the ones i remember about). Last version used -j1 for the tests, testing with that for now.

@kit-ty-kate
Copy link
Copy Markdown
Member

ok nevermind, it still fails with the same failure. Closing then.
I might have remembered old similar failures (e.g. #18002 (comment) or mirage/mirage-tcpip#435), I can't seem to find similar failures in my logs (though similar non-deterministic failures have been seen in the past)

sternenseemann added a commit to sternenseemann/nixpkgs that referenced this pull request Mar 27, 2021
This reverts commit 988f5a5.

The release process for many OCaml packages and in extension mirage
related packages usually entails creating a release in the respective
own repository so a release tarball becomes available and then opening a
PR against ocaml/opam-repository to finalize the release. During this
new issues can be discovered which push the release back.

This happened for mirage-tcpip 6.1.0 several times:
ocaml/opam-repository#18357
Prompting in total 3 different 6.1.0 releases with different hashes
respectively (the hash for ocamlPackages.tcpip.src shouldn't be
reproducible anymore, but we probably have cached the tarball already).
Ultimately the PR to opam-repository was closed to investigate some
failures on opam-repository's CI and the release postponed:
ocaml/opam-repository#18357 (comment)

I jumped the gun with the release and updated tcpip in nixpkgs before
tcpip was “properly” released in opam. I usually watch the github
repository of package I maintain for releases and can react pretty
quickly to a release as a result. Most of the time I also check
opam-repository's PRs nowadays for extra context or information, but
when everything seems fine and tests succeed I deem the update alright
to PR to nixpkgs. Being faster than opam was achievable in these cases
and actually seems kind of tantalizing.

In the light of this experience however, we should wait for the opam
PR getting merged at least for some packages that exhibit this behavior
of rereleasing the same version number multiple times to get the release
just right (afaik the 6.1.0 tag pointed to three different revisions for
tcpip). To me this is questionable upstream behavior we just have to deal
with in some way.
vbgl pushed a commit to NixOS/nixpkgs that referenced this pull request Mar 27, 2021
This reverts commit 988f5a5.

The release process for many OCaml packages and in extension mirage
related packages usually entails creating a release in the respective
own repository so a release tarball becomes available and then opening a
PR against ocaml/opam-repository to finalize the release. During this
new issues can be discovered which push the release back.

This happened for mirage-tcpip 6.1.0 several times:
ocaml/opam-repository#18357
Prompting in total 3 different 6.1.0 releases with different hashes
respectively (the hash for ocamlPackages.tcpip.src shouldn't be
reproducible anymore, but we probably have cached the tarball already).
Ultimately the PR to opam-repository was closed to investigate some
failures on opam-repository's CI and the release postponed:
ocaml/opam-repository#18357 (comment)

I jumped the gun with the release and updated tcpip in nixpkgs before
tcpip was “properly” released in opam. I usually watch the github
repository of package I maintain for releases and can react pretty
quickly to a release as a result. Most of the time I also check
opam-repository's PRs nowadays for extra context or information, but
when everything seems fine and tests succeed I deem the update alright
to PR to nixpkgs. Being faster than opam was achievable in these cases
and actually seems kind of tantalizing.

In the light of this experience however, we should wait for the opam
PR getting merged at least for some packages that exhibit this behavior
of rereleasing the same version number multiple times to get the release
just right (afaik the 6.1.0 tag pointed to three different revisions for
tcpip). To me this is questionable upstream behavior we just have to deal
with in some way.
@samoht
Copy link
Copy Markdown
Member

samoht commented Mar 30, 2021

It's not super surprising that this fails If concurrent tests try to open the same port. /cc @MagnusS

@MagnusS
Copy link
Copy Markdown
Contributor

MagnusS commented Mar 30, 2021

@samoht looks like it may be an old bug that didn't fail the tests before this release, I followed up in mirage/mirage-tcpip#446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants