Build with ghc(js) 9.8.2 + 9.10.1 #1093

ymeister · 2024-08-18T09:02:52Z

This PR mostly deals with building obelisk/lib libraries via ghc(js) 9.8.2 + 9.10.1, and not integrating new js backend into default.nix, yet. I intend to create a test app and based on that app recreate default.nix that uses haskell.nix instead of nixpkgs/reflex-platform (may backport some things into reflex-platform). Still, it passes selftest and is backwards compatible with previous ghc/ghcjs setup.

Built using haskell.nix/cabal-install.
Here's a "hello world" example on how to build a project depending on this version of obelisk: https://github.com/ymeister/obelisk-example

Needs:

I have:

Based work on latest develop branch
Followed the contribution guide
Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
Run the test suite: $(nix-build -A selftest --no-out-link)
Updated the changelog
(Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)

alexfmpe · 2025-01-30T11:47:44Z

.gitignore

@@ -14,5 +14,5 @@ cabal.project.local
 **/ghcid-output.txt

 # HLS specific files
-lib/cabal.project
+#lib/cabal.project


Why a comment?

So that git wouldn't ignore lib/cabal.project for cabal/haskell.nix builds.

Why not remove then?

Well somebody put it there for a reason I assume, if it causes problems in the future people would at least see a possible solution there.

alexfmpe · 2025-01-30T11:49:37Z

lib/asset/manifest/obelisk-asset-manifest.cabal

@@ -39,6 +40,9 @@ library
    -Wall -Werror -Wredundant-constraints -Wincomplete-uni-patterns -Wincomplete-record-updates -O2
    -fno-warn-unused-do-bind -funbox-strict-fields -fprof-auto-calls

+  if arch(javascript)
+    buildable: False


This seems right, but I'm confused on why we didn't need a if impl(ghcjs) before. Was it only implicitly gated by cabal.project ?

Not sure...

alexfmpe · 2025-01-30T11:53:06Z

lib/cabal.dependencies.project

+index-state: 2024-08-04T00:00:00Z
+allow-newer: all
+
+constraints:
+    hnix-store-core < 0.7
+  , hnix-store-remote < 0.7
+
+source-repository-package
+  type: git
+  location: https://github.com/ymeister/splitmix.git
+  tag: fe4d9e4ec01ba7caf8053d6888ec2e7f89fad874
+  --sha256: 19fbwcmdmb9w34cp19r2j4qywhnjmxxdv4rwci29pzbvgbnnjdia
+
+if !arch(javascript)
+  source-repository-package
+    type: git
+    location: https://github.com/ymeister/hs-git.git
+    tag: 4534c4589fc63d76d4a28f4ca9d810bea021964b
+    --sha256: 12c4llylc5zls85x11inkxdwllbps77pvwyji7jv9a8c069fg6sf
+
+  source-repository-package
+    type: git
+    location: https://github.com/mpickering/haskell-filesystem.git
+    tag: 2eb26717e986442796d703a80869e6826a10191e
+    subdir: system-fileio system-filepath
+    --sha256: sha256-VDShV+gkVUooMy1OtxrFfZrTAVVhWN/Ffjd6Qq0kHNM=
+
+  source-repository-package
+    type: git
+    location: https://github.com/ymeister/unix-compat.git
+    tag: 339649401c876ca1f76c6f94d6b099c8c47fa9e2
+    --sha256: 0j85q0mqg929nlz9ks6jaxz54pkvifpmldsvqjcz4bv6wml8m3wd


Do we still need these?

Yes, for splitmix. "Probably yes" for the others.

alexfmpe · 2025-01-30T11:53:59Z

lib/cabal.project

@@ -0,0 +1,23 @@
+if arch(javascript)


Can we pull the common ones out of the if ?
Also, should we remove the old cabal.project.dev ?

Can we pull the common ones out of the if ?

Yes. Wasn't aware that there could be multiple packages: statements that collect all of them at the time of writing.

Also, should we remove the old cabal.project.dev ?

I assume so. Didn't want to touch that as I was not 100% of cabal.project.dev goal.

Yes. Wasn't aware that there could be multiple packages: statements that collect all of them at the time of writing.

Me neither, I didn't even know we could use cabal conditionals in .project

I assume so. Didn't want to touch that as I was not 100% of cabal.project.dev goal.

I don't see it doing anything your cabal.project doesn't do and better.
I think it's bitrotted due to the bad ergonomics I ran into when doing the GHC 9.6 PR.
The libs had -Werror which was a pain for nix builds but the cabal.project was explicitly disabling that, contributing to the problem.

alexfmpe · 2025-01-30T11:57:07Z

lib/command/src/Obelisk/App.hs

-  ( CliConfig
-  , CliLog
-  , CliThrow
-  , CliT (..)
-  , ProcessFailure


Huh, were these all just unused?

alexfmpe · 2025-01-30T11:59:43Z

lib/command/src/Obelisk/Command.hs

-            "Make packages found in DIR available in the package database (but only when they are used dependencies). \
-            \ This will build the packages in DIR before loading GHCi. \
-            \See help for --interpret for how the two options are related."
+            "Make packages found in DIR available in the package database (but only when they are used dependencies). This will build the packages in DIR before loading GHCi. See help for --interpret for how the two options are related."


Do we know why these lines were split that way before?
I don't particularly mind this change, but in general let's avoid doing unrelated drive-by changes in PRs as long as we have a big PR queue of potential conflicts

I think multi-line string thing broke or changed is some way in newer ghc versions. Couldn't compile without this change.

Aaah it's some interaction of multiline strings with {-# LANGUAGE CPP #-}. I remember that biting me once.
I think there's actually a multiline string extension on 9.12 but we can't ditch older GHC yet so let's go with your change

alexfmpe · 2025-01-30T12:02:12Z

lib/command/src/Obelisk/Command/Project.hs

@@ -66,6 +65,11 @@ import System.PosixCompat.User
 import qualified System.Process as Proc
 import Text.ShellEscape (sh, bash, bytes)

+#if !MIN_VERSION_base(4,18,0)


Any reason we're using base here but used __GLASGOW_HASKELL__ >= 906 above?
Maybe it'd be clearer if we used MIN_VERSION_mtl(2,3,0)

Because the root of the error/warning is the change in the base library. Although ghc versions and base go hand-in-hand, I preferred addressing the root cause.

Oh, I thought this was the mtl re-export breakage in 2.3

alexfmpe · 2025-01-30T12:07:22Z

lib/route/src/Obelisk/Route.hs


 unpackTextEncoder :: (Applicative check, Applicative parse, IsText text) => Encoder check parse text String
-unpackTextEncoder = isoEncoder unpacked
+unpackTextEncoder = viewEncoder unpacked


Didn't I already do this in #1075?

Yes, this PR is built with parts of #1075 . I thought I cherry-picked the commits or at least marked you as a co-author, but apparently the commit history disagrees... You can force push necessary changes if you want or build a new PR using parts of this one if you want.

It's fine either way, I just don't understand how github produces this diff when both develop and the PR branch have viewEncoder

I think the diff is stuck at the old develop due to merge conflicts. Merging develop into this branch will probably resolve the issue.

alexfmpe · 2025-02-01T04:54:42Z

lib/command/obelisk-command.cabal

@@ -24,7 +24,7 @@ library
    , git
    , github
    , here
-    , hnix
+    , hnix >=0.5


I can't see anything else in the command diff related to this.
For future reference, was this already de-facto or needed for some other reason?

alexfmpe · 2025-02-01T05:02:40Z

lib/asset/manifest/obelisk-asset-manifest.cabal

@@ -24,6 +24,7 @@ library
    , filepath
    , template-haskell
    , text
+    , th-abstraction >= 0.6


$ ob run -v ... Configuring obelisk-asset-manifest-0.1... CallStack (from HasCallStack): $, called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:1024:20 in Cabal-3.2.1.0:Distribution.Simple.Configure configureFinalizedPackage, called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:477:12 in Cabal-3.2.1.0:Distribution.Simple.Configure configure, called at libraries/Cabal/Cabal/Distribution/Simple.hs:625:20 in Cabal-3.2.1.0:Distribution.Simple confHook, called at libraries/Cabal/Cabal/Distribution/Simple/UserHooks.hs:65:5 in Cabal-3.2.1.0:Distribution.Simple.UserHooks configureAction, called at libraries/Cabal/Cabal/Distribution/Simple.hs:180:19 in Cabal-3.2.1.0:Distribution.Simple defaultMainHelper, called at libraries/Cabal/Cabal/Distribution/Simple.hs:116:27 in Cabal-3.2.1.0:Distribution.Simple defaultMain, called at /nix/store/4mdp8nhyfddh7bllbi7xszz7k9955n79-Setup.hs:2:8 in main:Main Setup: Encountered missing or private dependencies: th-abstraction >=0.6

Actually this has already been handled in 0393bdf and bca0ad5

alexfmpe · 2025-02-01T15:02:48Z

lib/command/src/Obelisk/Command/Project.hs

@@ -61,7 +61,7 @@ import System.IO.Temp
 import System.IO.Unsafe (unsafePerformIO)
 import System.PosixCompat.Files
 import System.PosixCompat.Types
-import System.PosixCompat.User


I doubt we want to go out of our way to keep using an old version of unix-compat, for the sake of a single function which simply crashes: https://github.com/jacobstanley/unix-compat/pull/62/files#diff-93ed634b109420481984ea0fecc52a958c1d98c2728f8ec0b4c6ed741a8d4a24L55-L63

If ob survives long enough for windows support to be a concern, a lot of things will need to be revisited anyway.

alexfmpe · 2025-02-01T15:04:41Z

lib/cabal.dependencies.project

-    type: git
-    location: https://github.com/mpickering/haskell-filesystem.git
-    tag: 2eb26717e986442796d703a80869e6826a10191e
-    subdir: system-fileio system-filepath


I'm not sure why this was added but the upstream PR has been merged fpco/haskell-filesystem#27

alexfmpe · 2025-02-01T15:06:18Z

lib/command/src/Obelisk/Command.hs

+#if MIN_VERSION_nix_thunk(0,7,1)
+  <*> optional (strOption (short 'r' <> long "rev" <> metavar "REVISION" <> help "Update to this specific revision"))
+#endif


I backported for now, but shouldn't we simply be importing all these parsers from nix-thunk?
@ryantrinkle

alexfmpe · 2025-02-01T15:09:34Z

lib/command/src/Obelisk/Command/Deploy.hs

    Right (ThunkData_Packed _ ptr) -> return ptr
-    _ -> wrapNixThunkError (getThunkPtr CheckClean_NotIgnored root Nothing)
+    _ -> wrapNixThunkError $ compat $ getThunkPtr CheckClean_NotIgnored root Nothing
  deployInit' thunkPtr deployOpts
+  where
+#if MIN_VERSION_nix_thunk(0,7,1)
+    compat = fmap $ \(ptr, _isWorktree) -> ptr
+#else
+    compat = id
+#endif


I guess we should treat a worktree dir the same as a standalone unpacked dir?
@ryantrinkle

ymeister added 9 commits August 8, 2024 02:27

Build 'obelisk-executable-config-lookup' with ghc(js) 9.8 + 9.10

42228f9

Build 'tabulation' with ghc(js) 9.8 + 9.10

61853b4

Build 'obelisk-route' with ghc(js) 9.8 + 9.10

df0cdc2

Build 'obelisk-frontend' with ghc(js) 9.8 + 9.10

3381a03

Mark js incompatible libraries as non-buildable with js

730fc64

Build 'obelisk-asset-manifest' with ghc 9.8 + 9.10

9f23d8b

Build the rest of the libs with ghc 9.8 + 9.10

961f157

Split 'cabal.project'

b3fbfdd

Test with 'nix-build -A selftest'

e852cb2

ymeister requested review from ryantrinkle and ali-abrar August 18, 2024 09:03

alexfmpe reviewed Jan 30, 2025

View reviewed changes

alexfmpe reviewed Feb 1, 2025

View reviewed changes

alexfmpe added 10 commits February 1, 2025 11:07

Merge branch 'develop' into ghc(js)-9.10

2d46489

Fix unused import in older GHC

01308c0

Handle build failure in HsOpenSSL with recent openssl

9e410cb

Support nix-thunk >= 0.7.1

d799ab9

Bump index state

bfc3126

Remove unused git dependency

fcf3a0a

Remove dependency on forked unix-compat

4fb1b41

Remove since-merged override for system-fileio and system-filepath

e7668b0

Relax process lower bound

7de05e4

Make allow-newer more explicit

e27dcca

alexfmpe reviewed Feb 1, 2025

View reviewed changes

Simplify cabal.project

be031e3

alexfmpe force-pushed the ghc(js)-9.10 branch from 1f264b0 to 12f94a5 Compare February 1, 2025 15:19

alexfmpe force-pushed the ghc(js)-9.10 branch 5 times, most recently from a3e2aea to b97258d Compare February 1, 2025 15:59

Remove old project file

508ebb8

alexfmpe force-pushed the ghc(js)-9.10 branch 2 times, most recently from 965efc7 to 60b38d6 Compare February 1, 2025 17:04

Add github CI

769dd6f

alexfmpe force-pushed the ghc(js)-9.10 branch from 60b38d6 to 769dd6f Compare February 1, 2025 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build with ghc(js) 9.8.2 + 9.10.1 #1093

Build with ghc(js) 9.8.2 + 9.10.1 #1093

ymeister commented Aug 18, 2024

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025 •

edited

Loading

alexfmpe Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Jan 30, 2025

ymeister Jan 30, 2025

alexfmpe Feb 1, 2025

alexfmpe Feb 1, 2025

alexfmpe Feb 1, 2025 •

edited

Loading

alexfmpe Feb 1, 2025

alexfmpe Feb 1, 2025

alexfmpe Feb 1, 2025

alexfmpe Feb 1, 2025

Build with ghc(js) 9.8.2 + 9.10.1 #1093

Are you sure you want to change the base?

Build with ghc(js) 9.8.2 + 9.10.1 #1093

Conversation

ymeister commented Aug 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymeister Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfmpe Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymeister Jan 30, 2025 •

edited

Loading

alexfmpe Feb 1, 2025 •

edited

Loading