Fix compilation for wasm32-wasi #205

TerrorJack · 2022-04-14T15:41:13Z

This patch fixes compilation for wasm32-wasi. Much of the added CPP guards are not wasm-specific though, they merely check for headers and functions that exists on typical posix platforms, but not on wasm32-wasi.

Bodigrim · 2022-04-17T16:21:57Z

@TerrorJack please target master branch instead.

TerrorJack · 2022-04-19T11:58:04Z

Thanks, I've retargetted the PR against master.

Bodigrim · 2022-04-19T20:18:59Z

@TerrorJack build fails with

User.hsc:584:2: error: #endif without #if
#endif 
 ^
User.hsc:584:2: error: #endif without #if
#endif 
 ^
User.hsc:584:2: error: #endif without #if
#endif

TerrorJack · 2022-04-20T09:38:53Z

Ah, thanks for pointing that out. I've fixed it and unix-tests is passing now.

Bodigrim

Overall looks reasonable to me, but please add {-# WARNING #-} similar to #178.

unix.buildinfo.in

Bodigrim · 2022-04-20T18:17:52Z

@TerrorJack and rebase again please.

Bodigrim · 2022-04-22T20:30:35Z

This looks reasonable to me, but I'm out of my depth, so it is up to @hs-viktor to review.

hs-viktor · 2022-04-22T20:31:41Z

I am reading this today, looks reasonable so far, will be done soon.

hs-viktor

Overall the PR seems sound, but I have some concerns.

Perhaps "dummy" values could be returned in some cases rather than throwing an error. Errors might be deferred until these are used in some unsupported operation.

More fundamentally, I am concerned about the maintainability of this approach. As new functions are added or platforms evolve, it is not clear that we have adequate testing resources or platform knowledge of web assembly. This package is supposed to provide OS API bindings on POSIX systems, but clearly web assembly is not a POSIX platform.

Is liberally sprinkling "unix" with #ifdef guards the best way to support this platform?

hs-viktor · 2022-04-23T17:00:30Z

System/Posix/DynamicLinker/Prim.hsc

+#else
+
+{-# WARNING packRTLDFlag
+    "operation will throw 'IOError' \"unsupported operation\" (CPP guard: @#if HAVE_DLFCN_H@)" #-}
+packRTLDFlag _ = throw (ioeSetLocation unsupportedOperation "packRTLDFlag")
+
+#endif // HAVE_DLFCN_H


Another option is to simply return 0, presumably any meaningful operation on the returned flags will be unsupported, and fail, but I don't see a compelling reason to throw an error in this pure operation.

If there's consensus on this view, similar considerations might apply in a few other places in this PR...

Only some of the functions can return dummy values. I believe the extra effort to defer the errors for a small part of unix functions is not worth it, and adds a lot of cognitive overhead for reviewers.

Bodigrim · 2022-04-24T10:42:20Z

@TerrorJack could you possibly give us more perspective on your wasm32-wasi work? Is it expected to be merged into GHC 9.6? How can we test this branch? What we really need is a CI job for this arch, otherwise for every other PR, which adds or extends API, we'd have to ask you to validate it manually, which is no way to go.

Echoing Viktor's question, might it be better to have a separate library for WebAssembly platform, so that clients choose between unix, Win32 and, say, wasm32 packages?

I think that shifting unix focus from POSIX to non-Windows is not a show-stopper, but I'd appreciate more context.

hs-viktor · 2022-04-24T12:16:16Z

Echoing Viktor's question, might it be better to have a separate library for WebAssembly platform, so that clients choose between unix, Win32 and, say, wasm32 packages?

I think that shifting unix focus from POSIX to non-Windows is not a show-stopper, but I'd appreciate more context.

It may be appropriate to solicit views from @bgamari and other ghc-devs, since this PR becomes an architectural feature of GHC, not just a small tweak to some POSIX functions...

TerrorJack · 2022-04-25T18:31:50Z

Hi @hs-viktor @Bodigrim, thanks a lot for reviewing this patch and raising these questions!

For a bit more context about wasm32-wasi work: we did announce in ghc-devs, which links to the ghc wiki page WebAssembly backend. Hopefully the FAQ section in the wiki page contains some more info. tl;dr: yes it's happening, and we're going for 9.6.

The maintainability of this approach is totally a valid concern, but not as serious as it sounds:

We intend to take on the wasm-related maintainence burden, and not to force it on unix contributors. That means, PRs shall continue to be reviewed and merged based on testing on native arch/os.
In case there's breakage, which we'll only notice via ghc submodule bumps, we'll send PRs to fix. As you can see, the fixes are very straightforward!
Furthermore, almost all added autoconf/CPP guards are actually not wasm-specific, so they further benefit whoever needs to port GHC towards other exotic platforms.
unix is not fast evolving, based on hackage release history and commit history here. So the breakages won't be many.
The testing story can further be improved once upstream ghc-head supports wasm32-wasi codegen, and by then we can implement the CI job. It's even possible to add a CI job today, using our draft MR's working branch.

It also makes sense to ask, why not a wasi package in addition to unix or Win32? The answer is to maximize compatability with existing Haskell codebases out there: many if not all of them transitively depend on unix/Win32 already. If people can have unix in the wasm32-wasi global package database, have basic functionality working while the rest throws reasonable Haskell exception, then the UX will be better for all downstream users. It's also an approach chosen by ghcjs already.

Bodigrim · 2022-04-25T18:43:47Z

It's even possible to add a CI job today, using our draft MR's working branch.

That would be extremely useful.

Bodigrim · 2022-04-30T10:23:51Z

Cf. https://gitlab.haskell.org/ghc/ghc/-/issues/21426

We intend to take on the wasm-related maintainence burden, and not to force it on unix contributors. That means, PRs shall continue to be reviewed and merged based on testing on native arch/os.

I appreciate this, but it does not sound right to me. We might be making changes at the very last moment before release, and breaking one of deliverables would be most disappointing. @TerrorJack could you please create a CI job for wasm32-wasi build? Ideally on GHA infrastructure, feel free to smuggle a GHC bindist for wasm32-wasi branch from elsewhere.

TerrorJack · 2022-05-02T14:24:58Z

@Bodigrim Sure! Thanks for the kind words, I'm starting to work on the CI part.

Side note: there's a legacy ad hoc check that sets buildable to False when a header is not present. That check blocks this PR, and it's not used by either hadrian (picks unix/Win32 before invoking that check) or vanilla haskell packages (which use if os(windows) in their .cabal files). and if no objections, I can push a commit (or another PR) that removes that check.

Bodigrim · 2022-05-02T20:56:33Z

@TerrorJack please raise a separate PR.

TerrorJack · 2022-05-09T15:36:00Z

Hi all, sorry for the delay; this PR now includes a new CI job dedicated to wasm32-wasi build, which uses a bindist tarball from ghc gitlab. You're welcome to take a look and ask questions :)

Bodigrim · 2022-05-09T19:57:00Z

@TerrorJack there are plenty of build warning at https://github.com/haskell/unix/runs/6358300515?check_suite_focus=true#step:7:287, could you please clean them up? Disabling -Wno-unused-imports in affected modules and similar is fine.

TerrorJack · 2022-05-12T19:29:55Z

@Bodigrim The wasm32-wasi job compilation warnings have all been cleaned up. The only warning option that I needed to turn off globally was -Wunused-imports, since fixing them on a case-by-case basis would clutter the codebase a lot with #ifdefs around the imports.

TerrorJack · 2022-05-18T11:52:24Z

@Bodigrim 👀

hs-viktor · 2022-05-30T04:24:25Z

Where are we with this PR? I'm done with the technical review, but I still don't know whether we've resolved the maintenance burden issue.

P.S. I'm adding another test in #216 that expects openPseudoTerminal to work. Will that need to be conditional now?

Bodigrim · 2022-05-30T18:09:53Z

As discussed above, there is no expectation for unix maintainers to maintain compatibility with WebAssembly, if future developments interfere with it for reason or accidentally. I'm in favor of merging this, there is almost no cost for us to pay.

hs-viktor · 2022-05-30T18:30:52Z

Please rebase, there are new conflicts with Termina/Common.hsc as a result of the BaudRate changes.

hs-viktor · 2022-05-30T18:48:46Z

Please rebase, there are new conflicts with Termina/Common.hsc as a result of the BaudRate changes.

Note also that the new test may not be compatible with wasm.

TerrorJack · 2022-05-31T14:32:14Z

Thanks a lot for all the reviews; the PR has been rebased on recent BaudRate changes.

TerrorJack · 2022-06-02T17:54:05Z

Is there further action required to advance the state of this PR?

Bodigrim · 2022-06-02T19:26:08Z

Thanks @TerrorJack!

Bodigrim mentioned this pull request Apr 14, 2022

GHCJS patches. #111

Closed

TerrorJack changed the base branch from 2.7 to master April 19, 2022 11:57

Bodigrim reviewed Apr 20, 2022

View reviewed changes

unix.buildinfo.in Outdated Show resolved Hide resolved

TerrorJack requested a review from Bodigrim April 21, 2022 12:16

Bodigrim requested a review from hs-viktor April 21, 2022 21:10

hs-viktor reviewed Apr 23, 2022

View reviewed changes

TerrorJack mentioned this pull request May 3, 2022

Remove buildable check based on dlfcn.h #211

Closed

Bodigrim approved these changes May 30, 2022

View reviewed changes

TerrorJack added 4 commits May 30, 2022 23:24

Remove buildable check based on dlfcn.h

09e3fc5

Fix compilation for wasm32-wasi

0c41143

actually test wasm32-wasi linking

ad97e6e

Improve test

817c5df

Bodigrim merged commit e919c7d into haskell:master Jun 2, 2022

TerrorJack deleted the wasm32-wasi branch June 6, 2022 10:53

Fix compilation for wasm32-wasi #205

Fix compilation for wasm32-wasi #205

Uh oh!

Conversation

TerrorJack commented Apr 14, 2022

Uh oh!

Bodigrim commented Apr 17, 2022

Uh oh!

TerrorJack commented Apr 19, 2022

Uh oh!

Bodigrim commented Apr 19, 2022

Uh oh!

TerrorJack commented Apr 20, 2022

Uh oh!

Bodigrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bodigrim commented Apr 20, 2022

Uh oh!

Bodigrim commented Apr 22, 2022

Uh oh!

hs-viktor commented Apr 22, 2022

Uh oh!

hs-viktor left a comment

Choose a reason for hiding this comment

Uh oh!

hs-viktor Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

TerrorJack Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

Bodigrim commented Apr 24, 2022

Uh oh!

hs-viktor commented Apr 24, 2022

Uh oh!

TerrorJack commented Apr 25, 2022

Uh oh!

Bodigrim commented Apr 25, 2022

Uh oh!

Bodigrim commented Apr 30, 2022

Uh oh!

TerrorJack commented May 2, 2022

Uh oh!

Bodigrim commented May 2, 2022

Uh oh!

TerrorJack commented May 9, 2022

Uh oh!

Bodigrim commented May 9, 2022

Uh oh!

TerrorJack commented May 12, 2022

Uh oh!

TerrorJack commented May 18, 2022

Uh oh!

hs-viktor commented May 30, 2022

Uh oh!

Bodigrim commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hs-viktor commented May 30, 2022

Uh oh!

hs-viktor commented May 30, 2022

Uh oh!

TerrorJack commented May 31, 2022

Uh oh!

TerrorJack commented Jun 2, 2022

Uh oh!

Bodigrim commented Jun 2, 2022

Uh oh!

Uh oh!

Bodigrim commented May 30, 2022 •

edited

Loading