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

Mark eio_windows as only available on Windows #542

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Jun 2, 2023

It does install on other platforms, due to the (allow_empty) work-around for ocaml/dune#6938, but that's a bit confusing.

@talex5
Copy link
Collaborator Author

talex5 commented Jun 2, 2023

This will need an update to ocaml-ci so that it doesn't try testing Windows packages on Linux.

@avsm
Copy link
Contributor

avsm commented Jun 5, 2023

Doesn't this just make opam install . fail always since there's no coinstallable set? I'd much prefer that an opam installation of eio_windows always succeeds but installs a dummy package on other platforms. This in turn makes monorepo development far easier (if you merge this PR, then opam-monorepo will probably also run into issues)

@talex5
Copy link
Collaborator Author

talex5 commented Jun 5, 2023

Monorepo shouldn't be affected, since it doesn't install opam packages anyway (and certainly shouldn't be trying to install eio_windows on a non-Windows platform).

The installation instructions say to install just eio_main rather than trying to install everything, so they're also unaffected (though it would be nice if opam install . would filter out unavailable packages).

It would make much more sense to me for our tools to support the idea that different packages work on different platforms.

@avsm
Copy link
Contributor

avsm commented Jun 5, 2023

It would make much more sense to me for our tools to support the idea that different packages work on different platforms.

That's aspirational, and just not how the tools work right now. Monorepo will only happen to work since it clones the full eio repository that includes eio_windows.

What problem are you trying to solve with this PR? I find it less confusing if eio_windows just installs without error. We should reducing our reliance on the opam solver, not increasing it.

@talex5
Copy link
Collaborator Author

talex5 commented Jun 6, 2023

What problem are you trying to solve with this PR? I find it less confusing if eio_windows just installs without error.

The immediate problem is that, at the moment, releasing Eio to opam requires me to add the availability restriction manually each time, otherwise opam-repo-ci tests eio_windows on all the Linux platforms.

But more generally, I don't want to install broken packages. If a program depends on eio_windows and I try to install it on Linux, then I want it to fail immediately in the solver, telling me eio_windows isn't available. I don't want it to succeed, install an empty eio_windows package successfully, and then fail later with a confusing build error about missing modules.

We already do this with version constraints. ocaml-ci doesn't try to install Eio on OCaml 4.08 and then report success there because it successfully built a load of empty packages.

@avsm
Copy link
Contributor

avsm commented Jun 6, 2023

I could imagine us just having a single opam package (eio), and having optional submodules installed for the relevant operating system backends, and then pinning would work as well. The split is arbitrary, and we possibly seem to be packaging things up at the opam level at too fine a granularity.

I do think that OCaml version constraints (a core purpose of the opam solver) are fine; it's the other variables that have been added into the mix (OS/distro/arch) that I think should be handled at the build level, since they need to be handled there anyway. Why duplicate all that logic?

@talex5
Copy link
Collaborator Author

talex5 commented Jun 7, 2023

Building empty packages won't work in general. For example, if ocaml-ci starts testing on FreeBSD then we need to make sure it doesn't test eio_linux. Building an empty package won't even work, because it will try to install uring, which requires Linux. We'd have to remove the available tag from every package we depend on and make them all install empty packages too.

I don't see why adding correct availability information is a problem. What causes monorepo to fail with this?

@avsm
Copy link
Contributor

avsm commented Jun 9, 2023

What causes monorepo to fail with this?

The most obvious one is that if you use the solver to discriminate on variables other than OCaml version, then you are tied to the results of the evaluation of that variable for the lifetime of the monorepo. So if Windows packages aren't selected because you did the monorepo package calculation on Linux, it'll never build there. On the other hand, if you only pick one variable to select from in monorepo (OCaml version), and defer all the other decisions to build time (OS/arch/etc), then everything will just work. ocaml/dune#7899 might unblock some of the build-time side of what we need.

Currently it's entirely a coincidence that eio works with a monorepo due to everything existing in the same repo and being cloned as a unit by opam-monorepo.

@talex5
Copy link
Collaborator Author

talex5 commented Jun 20, 2023

In that case, it sounds like opam-monorepo should just ignore the available field when solving. Otherwise, any package in opam-repository could stop it working (e.g. uring already means you can't do a monorepo build on Windows). And probably the monorepo solver should be modified to fetch dependencies for all platforms, not just the one it's running on.

@avsm
Copy link
Contributor

avsm commented Jul 5, 2023

After a discussion with @talex5, I agree this particular shouldn't be blocked on the wider monorepo solver discussion!

@avsm avsm merged commit bd4cf9f into ocaml-multicore:main Jul 5, 2023
talex5 added a commit to talex5/eio that referenced this pull request Jul 8, 2023
This temporarily reverts ocaml-multicore#542 until the CI supports it.
@talex5
Copy link
Collaborator Author

talex5 commented Jul 8, 2023

I reverted this because the CI still needs to be updated to support it. I'll make another PR once that's done.

@talex5 talex5 deleted the available-windows branch July 8, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants