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

Remove no longer necessary MacOS RUSTFLAGS hack #9

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

lrascao
Copy link
Contributor

@lrascao lrascao commented Jan 16, 2021

The two PRs mentioned in the comments have since been merged, this can now be dropped.

@filmor
Copy link
Member

filmor commented Jan 20, 2021

The "hack" is still necessary, AFAICT, nothing changed in regard to this. The only thing that changed is that people are working on improving the syntax. The comments are probably outdated, though.

@lrascao
Copy link
Contributor Author

lrascao commented Jan 20, 2021

I noticed this when i bumped rust to 1.49, linking started failing on our tests at rebar3_cargo. Not too sure on the exact root cause for the failure but this is the way i found of going around it.

@belltoy
Copy link

belltoy commented Jun 15, 2021

Hi, guys. Since rust-lang/rust#36574 had been merged, the hack was also outdated. We still need to specify RUSTFLAGS for different targets, but it can be done via .cargo/config.toml instead of hard-coded hack like that. e.g:

# .cargo/config.toml
[target.'cfg(target_os = "macos")']
rustflags = [
    "-C", "link-arg=-undefined",
    "-C", "link-arg=dynamic_lookup",
]

So, this PR can be merged IMO.

@belltoy
Copy link

belltoy commented Jul 4, 2021

According to rusterlium/rustler#192 and the Apple reference, the two-level namespace is preferred, and:

When building executables with a two-level namespace, you can allow the remaining undefined symbols to be looked up by the dynamic linker if the program is targeted for OS X v10.3 and later (the MACOSX_DEPLOYMENT_TARGET environment variable is set to 10.3 or higher). To take advantage of this feature, use the -undefined dynamic_lookup option.

So with two-level namespace args in my example lib:

$ otool -hV priv/crates/my_nif/0.1.0/debug/libmy_nif.so
priv/crates/my_nif/0.1.0/debug/libmy_nif.so:
Mach header
      magic  cputype cpusubtype  caps    filetype ncmds sizeofcmds      flags
MH_MAGIC_64   X86_64        ALL  0x00       DYLIB    16       2064   NOUNDEFS DYLDLINK TWOLEVEL NO_REEXPORTED_DYLIBS MH_HAS_TLV_DESCRIPTORS

Meanwhile, the preferred for rust/cargo is to put RUSTFLAGS in the cargo config file.

@filmor cc @lrascao

@lrascao
Copy link
Contributor Author

lrascao commented Jul 5, 2021

i'll take care of the merge conflict

The two PRs mentioned have since been merged, this can now be dropped.
@filmor filmor merged commit 99c224d into rusterlium:master Jul 5, 2021
@belltoy
Copy link

belltoy commented Jul 5, 2021

Great! And should bump a new version to fix rusterlium/rebar3_cargo#6 which may should be update to using the new fix with a cargo config file?

@lrascao lrascao deleted the fix/rustflags_hack branch July 5, 2021 17:10
davisp added a commit to davisp/rust-nif-examples that referenced this pull request Jul 8, 2021
However, this requires manaully patching the `erlang-cargo` dependency
of the rebar3_cargo plugin to include this change:

rusterlium/erlang-cargo#9
@belltoy
Copy link

belltoy commented Jul 4, 2022

@filmor Could you bump a new release? And have a review at rusterlium/rebar3_cargo#8 if you have time. 🙂

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.

3 participants