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

Fix build on aarch64-darwin #1789

Merged
merged 5 commits into from
May 3, 2022
Merged

Conversation

jdahm
Copy link
Contributor

@jdahm jdahm commented Mar 11, 2022

Removes -Clink-arg=-Wl,--no-rosegment and adds -mno-outline-atomics and stackprotector on Darwin and aarch64.

To do:

  • decide whether to enable -Wl,--no-rosegment on non-aarch64 arches.

@the-mikedavis
Copy link
Member

This is looking good to me on x86_64-linux 👍

Does this fix the flake install for you, or just cargo build from within the nix develop shell?

@jdahm
Copy link
Contributor Author

jdahm commented Mar 11, 2022

Not quite... but it's so close! So far only fixes the cargo build in the nix develop shell. The flake build errors with:

error: builder for '/nix/store/bhx9cbs89vnvr2cxlvlwan2n2z30wh3p-helix-term-0.6.0.drv' failed with exit code 101;
       last 10 log lines:
       >           ld: warning: could not create compact unwind for _ts_node__prev_sibling.constprop.0: register 73 saved somewhere other than in frame
       >           Undefined symbols for architecture arm64:
       >             "___aarch64_ldadd4_acq_rel", referenced from:
       >                 _ts_subtree_retain in libtree_sitter-64cf1c9f9303e665.rlib(lib.o)
       >                 _ts_subtree_release in libtree_sitter-64cf1c9f9303e665.rlib(lib.o)
       >           ld: symbol(s) not found for architecture arm64
       >           clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

-mno-outline-atomics removed this link issue when using cargo in the development shell. So, it seems like the flake build isn't picking up that environment variable. Do you know how to add custom flags to the cargo build?

@jdahm
Copy link
Contributor Author

jdahm commented Mar 11, 2022

There are a few methods as mentioned in the docs, including adding it to the configuration for a specific target here https://doc.rust-lang.org/cargo/reference/config.html#configuration-format. Another option is setting that as an environment variable, which seems less "pure" in the nix sense. What do you think?

@the-mikedavis
Copy link
Member

I would probably experiment with an environment variable (you should be able to set one within helix-term like we do here), but the cargo config might be ideal so other packagers don't have to deal with the same problem.

@archseer
Copy link
Member

Can we still enable -Clink-arg=-Wl,--no-rosegment on Linux? I added this in order to get proper flamegraph stack traces: https://github.com/flamegraph-rs/flamegraph#cargo-flamegraph

@jdahm
Copy link
Contributor Author

jdahm commented Mar 17, 2022

Yes, I can definitely add that back. I didn't know the reason behind adding it, but that makes sense to continue doing that while it's required for LLD. Reference: flamegraph-rs/flamegraph#157.

I'll try to find time to wrap this up this week.

@jdahm
Copy link
Contributor Author

jdahm commented Mar 30, 2022

I found that telling nix-cargo-integration to use clang on MacOS fixes almost all the problems without having to hack custom flags. My current issue is figuring out how to set packageMetadata.nix.cCompiler = clang (or equivalent setting in Cargo.toml) when the system is darwin. Discussing that over in yusdacra/nix-cargo-integration#65.

@jdahm
Copy link
Contributor Author

jdahm commented Apr 15, 2022

nix run . is giving me a strange error on this branch: error: Cannot find source for helix-term:0.6.0. @the-mikedavis have you seen that one?

@the-mikedavis
Copy link
Member

Hmm I see the same when doing nix build on this branch. I haven't seen that elsewhere though. It looks from the trace like it's from within dream2nix, maybe it's something to do with the update to nix-cargo-integration?

@yusdacra
Copy link
Contributor

I can build this branch on the latest nix-cargo-integration (on a x86_64-linux machine, though). Whatever the issue related to sources was, it seems to be fixed.

@jdahm
Copy link
Contributor Author

jdahm commented Apr 30, 2022

@yusdacra thanks for checking that! I guess that means we should get this merged. I'm on vacation right now and away from my computer, so I can't get to this until next week. I gave you access to my fork in case you'd rather do that sooner.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch builds for me (x86_64-linux) and when rebased on master. Let's merge as-is since it's a nice bump of the dependencies and the changes aren't scary.

Could you give this a try on your M1 machine @jdahm? No rush of course, please enjoy your vacation 😀

@the-mikedavis the-mikedavis merged commit e529ca1 into helix-editor:master May 3, 2022
@the-mikedavis
Copy link
Member

Thanks for your help here and in nix-cargo-integration @yusdacra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants