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

Fix support for GHCJS_BROWSER compilation flag of GHC #333

Closed

Conversation

Swordlash
Copy link

See https://gitlab.haskell.org/ghc/ghc/-/issues/25613#note_602248

When compiling ghc under GHCJS_BROWSER, all the environment checking code should be removed by the preprocessor. This is not the case for the jsbits of the unix library, which causes troubles for Google Closure Compiler and bundlers. This PR adds missing directives.

@Swordlash
Copy link
Author

I was pushing code from gitlab, there seem to be some ghost commits in the history...

function h$js_futimes(fd,atime,mtime) {
#ifdef GHCJS_BROWSER
throw "h$js_futimes unsupported";
#else
if (!h$isNode()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be

Suggested change
if (!h$isNode()) {
if (!h$isNode() || defined(GHCJS_BROWSER)) {

?

Copy link
Author

@Swordlash Swordlash Jan 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, we want a preprocessor-level if, not a runtime-level if here; sources are preprocessed via emcc before compilation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words if ghc is compiled with -DGHCJS_BROWSER then h$isNode() is undefined at runtime and we don't want to include this code at all.

@Bodigrim
Copy link
Contributor

Bodigrim commented Jan 1, 2025

Please rebase cleanly on master, ignoring whatever commits were accumulated in GHC mirror.

@hasufell
Copy link
Member

hasufell commented Jan 6, 2025

ping @Swordlash

@hasufell hasufell force-pushed the wip/swordlash/ghcjs_browser_fix branch from f0bebdd to b9b134d Compare January 6, 2025 08:53
@hasufell hasufell requested a review from hsyl20 January 6, 2025 08:53
@hsyl20
Copy link
Contributor

hsyl20 commented Jan 6, 2025

LGTM.

@luite Could you review it too please?

@Swordlash
Copy link
Author

Thanks @hasufell :)

@hasufell hasufell requested a review from luite January 6, 2025 09:58
@Swordlash
Copy link
Author

I'm going to close this PR for now since we agreed to remove the macro altogether.

@Swordlash Swordlash closed this Jan 8, 2025
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.

4 participants