-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Android Cross compilation #35451
Android Cross compilation #35451
Conversation
Success on aarch64-linux (full log) Partial log (click to expand)
|
Success on x86_64-darwin (full log) Partial log (click to expand)
|
Success on x86_64-linux (full log) Partial log (click to expand)
|
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.
Minor nits, but looks good!
config = "aarch64-unknown-linux-android"; | ||
arch = "aarch64"; | ||
platform = platforms.aarch64-multiplatform; | ||
useAndroidPrebuilt = true; |
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.
Would be good to document somewhere what this means.
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.
Any idea where? The lib.system
infra today currently doesn't offer for such ad-hoc top-level ones. And let's do the same for your isiPhoneSimulator
too :D.
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.
At the very least in the cross-compiling manual?
@@ -1,5 +1,6 @@ | |||
{ fetchurl, stdenv, lib | |||
, buildPlatform, hostPlatform | |||
, androidMinimal ? stdenv.hostPlatform.useAndroidPrebuilt |
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.
Nothing android specific about it, maybe give it a better name?
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.
Yeah these two hacky (in that they do a bunch of mutually-unrelated random things) androidMinimal
options were cargo-cults on my part. I've just been lacking the time to go back and empirically derive what's actually needed. Kinda want a "mea culpa style" name for that reason to acknowledge the tech debt, but I'm open to suggestions.
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.
lightweight
or minimal
? Or if it makes more sense then maybe expose each change individually?
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.
Visual review, haven't tried yet. Awesome to see this, thanks!
Shouldn't there be an RFC about this first?
configureFlags = | ||
lib.optional stdenv.isFreeBSD "--with-pic"; | ||
configureFlags = lib.optional stdenv.isFreeBSD "--with-pic" | ||
++ lib.optionals androidMinimal [ "--enable-static" "--without-cxx" ]; |
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 don't think --without-cxx
is a valid configure flag for libiconv, please remove if it isn't.
#"--without-debug" | ||
#"--without-termlib" | ||
#"--without-ticlib" | ||
"--without-cxx" |
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.
Probably want to remove all these commented-out lines, and de-duplicate what remains.
Just curious, why "--without-cxx"?
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.
literally cargo culting some prior GHC on android work a year ago as per my comment above #35451 (comment), haha. I was too busy hacking up the first draft of everything we've merged since to look into this.
libc = targetAndroidndkPkgs.libraries; | ||
extraBuildCommands = lib.optionalString (targetPlatform.config == "arm-unknown-linux-androideabi") '' | ||
sed -E \ | ||
-i $out/bin/${targetPlatform.config}-cc \ |
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.
Doesn't hurt anything, but no reason to specify "-i" repeatedly. But keep it if you like it aesthetically.
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.
oh I meant it to be -e
repeatedly.
Also, does this work for you? Trying to build coreutils:
Similar for ncurses, and such. |
#"--without-debug" | ||
#"--without-termlib" | ||
#"--without-ticlib" | ||
"--without-cxx" |
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.
Are these comments and duplicated --without-cxx
important or can we clean this up?
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.
It's only important insofar that it functions as a TODO.
6378913
to
4d8254b
Compare
@dtzWill Fixed a thing so it actually builds |
Expose as an option for the cross stdenv.
4d8254b
to
2b896f9
Compare
2b896f9
to
3a672cb
Compare
Fixed and minimized the flags. @shlevy I'm going to document in a follow-up PR; It's possible I'll want to tweak how such ad-hoc attributes are used for a bit too, so the |
Motivation for this change
I need to make sure GHC still works, but hydra is currently down.
Will be used by http://github.com/reflex-frp/reflex-platform
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)CC @bgamari @ElvishJerricco