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

Does not build on GHCJS #40

Closed
saulzar opened this issue Oct 7, 2018 · 15 comments
Closed

Does not build on GHCJS #40

saulzar opened this issue Oct 7, 2018 · 15 comments
Labels

Comments

@saulzar
Copy link

@saulzar saulzar commented Oct 7, 2018

Is it meant to? (ghcjs 0.2.0, ghc 8.0.2)

src/TextShow/Data/Text.hs:25:1: error:
Failed to load interface for ‘Data.Text.Foreign’

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Oct 7, 2018

Hi @saulzar. I've never been able to successfully build text-show with GHCJS (or most large Haskell projects, for that matter), so I'm certainly not surprised to learn that text-show doesn't build with GHCJS. If you know how to fix the issue, I'd gladly accept a patch.

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Oct 7, 2018

OK, with the help of the folks at the #ghcjs IRC channel, I was able to get further than before. In fact, I was able to build the entirety of text-show-3.7.4 (the library, at least) with ghcjs-8.4.0.1, so perhaps this is simply an issue with an old version of ghcjs? This means I'd likely need to patch around the issue with some sort of CPP, but I don't know what that would be. If anyone reading this is familiar with ghcjs and knows the answer, your input would be appreciated.

@saulzar

This comment has been minimized.

Copy link
Author

@saulzar saulzar commented Oct 9, 2018

Hmm. I don't quite understand the error, maybe it's got nothing to do with text-show. Looks like that module should be present - but the interface files are missing in my environment (I'm using the reflex-platform).

In this enviornment the text version is 1.2.2.1 which definitely has this module, and I can import it just fine compiling with ghcjs --make.

@tgass

This comment has been minimized.

Copy link

@tgass tgass commented Mar 15, 2019

I managed to build a project using text-show without any problems using stack and ghcjs-0.2.1.9008011_ghc-8.0.2. But as I'm switching to reflex-platform I ran into the described problem as well. So I'd assume it's a problem on the reflex-platform side.

@tgass

This comment has been minimized.

Copy link

@tgass tgass commented Mar 15, 2019

There's now an issue in the reflex-platform project: reflex-frp/reflex-platform#466

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Mar 16, 2019

OK. I'm not familiar with how reflex-platform does things. Is the necessary patching that has to be done to text-show something that can be upstreamed into this repo?

@tgass

This comment has been minimized.

Copy link

@tgass tgass commented Mar 16, 2019

My current understanding is that nothing needs to be done in text-show but only in reflex-platform. text-show as it is compiles with ghcjs.

reflex-platform (when used with ghcjs) uses a fork of the text package that doesn't expose Data.Text.Foreign. And within reflex-platform there is already a place to adjust packages that are dependent on the forked text package. I'm currently looking into this and hopefully get a PR ready soon.

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Mar 16, 2019

Are these patches tied to particular Hackage releases? My concern is that as text-show makes new releases, this reflex-platform patch will quickly become out-of-date and have to be maintained separately.

@tgass tgass mentioned this issue Mar 17, 2019
@tgass

This comment has been minimized.

Copy link

@tgass tgass commented Mar 17, 2019

Here is the PR I made for reflex-platform (reflex-frp/reflex-platform#468). Ryan from the reflex project is asking if you'd be interested in taken it. Maybe just have a look.

@ryantrinkle

This comment has been minimized.

Copy link

@ryantrinkle ryantrinkle commented Mar 17, 2019

@RyanGlScott I think it's really on reflex-platform to make this work, but if you would be willing to have a flag that chops out the Foreign stuff, that would definitely make our lives easier!

For what it's worth, the reason we use a non-standard text package is that, in our test cases, it saved almost 50% of the total memory of a running program compared with the standard text implementation. Browsers are much better at dealing with their own strings than at letting us reimplement them in terms of typed buffers!

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Mar 17, 2019

Judging from the patch, it looks like the only part that's giving reflex-platform trouble is the I16 instance? To be honest, I have no particular reason for having that; I just blindly added it in an attempt to provide a TextShow instance to mirror every publicly accessible Show instance in the text. But I16 is really an internal component of text, and given a choice between workaround around the issue with CPP and just removing the I16 stuff altogether from text-show, I'm inclined to pick the latter (and wait until someone complains before adding it back). Does that sound like a reasonable course of action?

RyanGlScott added a commit that referenced this issue Mar 17, 2019
The usefulness of this instance was dubious at best, and
moreover, it was actively causing issues when building
`text-show` with `reflex-platform` (see #40).
@ryantrinkle

This comment has been minimized.

Copy link

@ryantrinkle ryantrinkle commented Mar 19, 2019

@RyanGlScott Thanks! I really appreciate you sorting this out :)

@tgass Want to strip out the patch in reflex-platform?

@tgass

This comment has been minimized.

Copy link

@tgass tgass commented Mar 19, 2019

@ryantrinkle Sure. I'll just revert the commit and open a PR.

@RyanGlScott

This comment has been minimized.

Copy link
Owner

@RyanGlScott RyanGlScott commented Mar 19, 2019

I've uploaded text-show-3.8 to Hackage with these changes.

@ryantrinkle

This comment has been minimized.

Copy link

@ryantrinkle ryantrinkle commented Mar 19, 2019

@RyanGlScott Awesome; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.