-
Notifications
You must be signed in to change notification settings - Fork 1
Multi target support #35
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
base: wip/make-build-pre-zig
Are you sure you want to change the base?
Multi target support #35
Conversation
5158146
to
8b1e18e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'll need to overhaul this program completely at some point.
This change looks good. I've left a few comments with ideas.
Oof, CI also tells us we don't really test this program at all on its own, just as part of the whole 😅 nothing to fix now, just an observation. |
mt <- getTarget my_flags | ||
dir <- case mt of | ||
Nothing -> pure dir' | ||
Just target -> pure (dir' </> "targets" </> target </> "lib") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path should be kept in sync with ghc: perhaps add a helper function where getBaseDir
is? If it's not possible because of staging, some comment would be useful to remember this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in GHC is a bit different:
Lines 142 to 161 in 1c9db51
targettopdir <- Just <$> do | |
topdir <- findTopDir mbMinusB | |
let targets_dir = topdir </> "targets" | |
-- list targets when asked | |
when list_targets $ do | |
putStrLn $ "Installed targets (in " ++ targets_dir ++ "):" | |
doesDirectoryExist targets_dir >>= \case | |
True -> do | |
ds <- listDirectory targets_dir | |
forM_ ds (\d -> putStrLn $ " - " ++ d) | |
False -> pure () | |
exitSuccess | |
-- otherwise select the appropriate target | |
case mbTarget of | |
Nothing -> pure topdir | |
Just target -> do | |
let r = targets_dir </> target </> "lib" | |
doesDirectoryExist r >>= \case | |
True -> pure r | |
False -> throwGhcException (UsageError $ "Couldn't find specific target `" ++ target ++ "' in `" ++ r ++ "'") |
What did you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about something like getTargetDir :: FilePath -> String -> IO FilePath
taking the topdir and the target name and returning the target dir.
As well as being invoked via triplet symlink.
8b1e18e
to
2de8dce
Compare
Instructions to test:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 This is great work! I've left a few comments with throughts.
AutoApply_V32.cmm | ||
AutoApply_V64.cmm | ||
|
||
common ghcjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be called rts-javascript
, or rts-ghcjs
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To align with the other common stanzas: rts-js-sources
being the best?
AutoApply_V32.cmm | ||
AutoApply_V64.cmm | ||
|
||
common ghcjs | ||
import: rts-base-config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to deviate from how other common stanzas are done? This should just be in the conditional below: rts-base-config, rts-ghcjs
, or similar.
visibility: public | ||
build-depends: rts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! This is an oversignt, and already part of rts-base-config
.
cabal.project.stage2
Outdated
utils/genprimopcode | ||
utils/deriveConstants | ||
|
||
if !arch(javascript) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !arch(javascript) | |
-- We do not want to compile the utilities to javascript. Also terminfo (and haskeline) don't make sense for javascript. | |
if !arch(javascript) |
cabal.project.stage2
Outdated
if arch(javascript) | ||
constraints: | ||
Cabal source, | ||
Cabal-syntax source, | ||
array source, | ||
base source, | ||
binary source, | ||
bytestring source, | ||
containers source, | ||
deepseq source, | ||
directory source, | ||
exceptions source, | ||
file-io source, | ||
filepath source, | ||
hpc source, | ||
integer-gmp source, | ||
mtl source, | ||
os-string source, | ||
parsec source, | ||
pretty source, | ||
process source, | ||
rts source, | ||
rts-headers source, | ||
rts-fs source, | ||
stm source, | ||
system-cxx-std-lib source, | ||
template-haskell source, | ||
text source, | ||
time source, | ||
transformers source, | ||
unix source, | ||
xhtml source, | ||
Win32 source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation is meant to signify dependencies?
Why do we need the source constraint here? We should have acomment explaining why this is done, and if this is a cabal defect that needs to be rectified or something else.
$(call run_and_log, HADRIAN_SETTINGS='$(HADRIAN_SETTINGS)' \ | ||
PATH=$(PWD)/_build/stage1/bin:$(PATH) \ | ||
$(CABAL_BUILD) --ghc-options="-ghcversion-file=$(abspath ./rts/include/ghcversion.h)" -W $(GHC0) rts:nonthreaded-nodebug ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really feels like a bug, why do we need to build this explicitly here, why is this not brought into scope automatically?
Makefile
Outdated
javascript-unknown-ghcjs-libs: private CC=emcc | ||
javascript-unknown-ghcjs-libs: _build/stage2/bin/javascript-unknown-ghcjs-ghc-pkg _build/stage2/bin/javascript-unknown-ghcjs-ghc _build/stage2/lib/targets/javascript-unknown-ghcjs/lib/settings _build/stage2/lib/targets/javascript-unknown-ghcjs/bin/unlit _build/stage2/lib/targets/javascript-unknown-ghcjs/lib/package.conf.d | ||
# Force cabal to replan | ||
rm -rf _build/stage2/javascript-unknown-ghcjs/cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this seems ... odd? @andreabedini what's going on here?
Makefile
Outdated
rm -rf _build/stage2/javascript-unknown-ghcjs/cache | ||
$(call run_and_log, HADRIAN_SETTINGS='$(HADRIAN_SETTINGS)' \ | ||
PATH=$(PWD)/_build/stage2/bin:$(PATH) \ | ||
$(CABAL_BUILD) -W $(GHC2) --happy-options="--template=$(abspath _build/stage2/src/happy-lib-2.1.5/data/)" rts:nonthreaded-nodebug ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember what the underlying reason for the happy-lib-... path thing was? Can we add a comment to remember why this was needed?
Makefile
Outdated
$(CABAL_BUILD) -W $(GHC2) --happy-options="--template=$(abspath _build/stage2/src/happy-lib-2.1.5/data/)" rts:nonthreaded-nodebug ) | ||
$(call run_and_log, HADRIAN_SETTINGS='$(HADRIAN_SETTINGS)' \ | ||
PATH=$(PWD)/_build/stage2/bin:$(PATH) \ | ||
$(CABAL_BUILD) -W $(GHC2) --happy-options="--template=$(abspath _build/stage2/src/happy-lib-2.1.5/data/)" $(STAGE2_TARGET_LIBS) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't the STAGE2_CROSS_TARET_LIBS
contain rts:nonthreaded-nodebug
? If not, why not?
Makefile
Outdated
@@ -176,6 +182,33 @@ STAGE2_UTIL_EXECUTABLES := \ | |||
runghc \ | |||
unlit | |||
|
|||
STAGE2_TARGET_LIBS := \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the CROSS compiler, right? Can we rename it to STAGE2_CROSS_TARET_LIBS then?
I've stage3-ified this and added musl cross compiler support. |
Updated build instructions afaict: Instructions to test:
These instructions worked for me. Note that replacing Then it fails with:
that someone reported (again? I've already seen it) 2 days ago: https://gitlab.haskell.org/ghc/ghc/-/issues/26290 |
As well as being invoked via triplet symlink.