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

androidenv: fix autopatching toolchains #216142

Merged
merged 1 commit into from
Apr 1, 2023

Conversation

jakubgs
Copy link
Contributor

@jakubgs jakubgs commented Feb 13, 2023

Description of changes

Use of binaries from NDK toolchains has been broken by following PR:

I'm splitting the patchInstructions to run the ELF patching only on Linux.

[nix-shell:~/work/status-mobile]$ $ANDROID_HOME/ndk/22.1.7171670/toolchains/x86_64-4.9/prebuilt/linux-x86_64/bin/x86_64-linux-android-strip --help
bash: /nix/store/084pp923k5k24gnb44h9d6azk84grnh8-androidsdk-mod-sdk/ndk/22.1.7171670/toolchains/x86_64-4.9/prebuilt/linux-x86_64/bin/x86_64-linux-android-strip: No such file or directory

Related to another fix of bug introduced by #195752:

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@jakubgs jakubgs mentioned this pull request Feb 13, 2023
13 tasks
@jakubgs jakubgs force-pushed the androidenv/fix-toolchains branch from e57733e to d4f134e Compare February 13, 2023 09:56
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 13, 2023
@wegank
Copy link
Member

wegank commented Feb 13, 2023

andoridenv -> androidenv

@jakubgs jakubgs changed the title andoridenv: fix autopatching toolchains androidenv: fix autopatching toolchains Feb 13, 2023
@jakubgs jakubgs force-pushed the androidenv/fix-toolchains branch from d4f134e to 2a14c00 Compare February 13, 2023 17:25
@sheldonneuberger-sc
Copy link
Contributor

Sorry for breaking this. The reason for the change is that autoPatchelf won't work on mac. I think we can make both linux and mac work by either:

  1. Gate this part of the script to only run on linux, or
  2. Implicitly make it skip darwin dirs (like /nix/store/lzvf8r573yqsi409s86qkn4kil8sr7j8-ndk-23.0.7599858/libexec/android-sdk/ndk/23.0.7599858/toolchains/llvm/prebuilt/darwin-x86_64)

@jakubgs
Copy link
Contributor Author

jakubgs commented Feb 13, 2023

How exactly does it break it?

@sheldonneuberger-sc
Copy link
Contributor

sheldonneuberger-sc commented Feb 13, 2023

On mac it gives this error autoPatchelf: command not found. This would fix it, just move the autoPatchelf sections into an isLinux section (I had originally put up a PR that did this, but IIRC someone preferred not having "if isLinux" logic in the script, not sure why).

efe9215

@jakubgs
Copy link
Contributor Author

jakubgs commented Feb 13, 2023

You know you can just link to a diff on GitHub? Also, you can add highlights to a diff by writing diff after first triple backticks.

My question would be, which steps are necessary on Macos? Because those could just be extracted out into a separate phase that would the be called by runHook.

@jakubgs
Copy link
Contributor Author

jakubgs commented Feb 13, 2023

Also, if the issue is just missing patchelf, why not just include it?

nativeBuildInputs = [ makeWrapper ]
++ lib.optionals stdenv.isLinux [ autoPatchelfHook ];

Maybe I'm missing something, but what's stopping us from just doing:

  nativeBuildInputs = [ makeWrapper autoPatchelfHook ];

@wegank
Copy link
Member

wegank commented Feb 13, 2023

Maybe I'm missing something, but what's stopping us from just doing:

  nativeBuildInputs = [ makeWrapper autoPatchelfHook ];

macOS executables aren't in ELF format, so autoPatchelfHook doesn't apply.

@sheldonneuberger-sc
Copy link
Contributor

autoPatchelfhook can't be built on mac. I missed one of the elf sections, here's an updated diff efe9215. AFAIK, all of the other patches should still be done on mac.

@jakubgs jakubgs force-pushed the androidenv/fix-toolchains branch from 2a14c00 to bf628ce Compare February 13, 2023 21:26
@jakubgs jakubgs force-pushed the androidenv/fix-toolchains branch 2 times, most recently from 71533ae to 0d6f6fe Compare February 13, 2023 21:49
Use of binaries from NDK `toolchains` has been broken by following PR:

* NixOS#195752

I'm splitting the patchInstructions to run the ELF patching only on Linux.

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@jakubgs jakubgs force-pushed the androidenv/fix-toolchains branch from 0d6f6fe to 93e9aac Compare February 14, 2023 15:12
@jakubgs
Copy link
Contributor Author

jakubgs commented Feb 22, 2023

@SuperSandro2000 any chance we could get this merged?

@SuperSandro2000 SuperSandro2000 merged commit 7ecf564 into NixOS:master Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants