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

rust: add hasLibraries and hasIncludes #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbigras
Copy link

@bbigras bbigras commented Jun 2, 2021

See #56 (comment) maybe

Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

Sounds good on principle. It looks a lot like the language.c module so maybe it's better to import that instead. @Mic92 also said that LD_LIBRARY_PATH means that the binary won't work outside of the devshell if I am not mistaken.

libraries = mkOption {
type = types.listOf strOrPackage;
default = [ ];
description = "Use this when another language dependens on a dynamic library";
Copy link
Member

Choose a reason for hiding this comment

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

Typo, but I still don't understand what this means.

Suggested change
description = "Use this when another language dependens on a dynamic library";
description = "Use this when another language depends on a dynamic library";

What would be another language? Can you make an example.

Choose a reason for hiding this comment

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

I think "another language" isn't really necessary (and a holdover from the equivalent C option), this should really be for any non-crate library dependencies (i.e. anything that would need pkg-config).

includes = mkOption {
type = types.listOf strOrPackage;
default = [ ];
description = "Rust dependencies from nixpkgs";
Copy link
Member

@Mic92 Mic92 Jun 3, 2021

Choose a reason for hiding this comment

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

Suggested change
description = "Rust dependencies from nixpkgs";
description = "Dependencies on C headers to provided by nixpkgs";

}];
}]
++
(lib.optionals hasLibraries [
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want to make the separation into libraries/includes? Would it not be easier to have just one package options that adds both?

Choose a reason for hiding this comment

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

Since includes were for C and specifically C_INCLUDE_PATH, I don't see how it's necessary to port that to the rust options.

prefix = "$DEVSHELL_DIR/include";
}
{
name = "PKG_CONFIG_PATH";
Copy link
Member

Choose a reason for hiding this comment

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

Don't libraries also depend on PKG_CONFIG_PATH some time?

@bbigras
Copy link
Author

bbigras commented Jun 3, 2021

@Mic92
Copy link
Member

Mic92 commented Jun 3, 2021

@bbigras
Copy link
Author

bbigras commented Jun 3, 2021

@Mic92 also said that LD_LIBRARY_PATH means that the binary won't work outside of the devshell if I am not mistaken.

Does it differ from using a "normal" nix-shell with cargo as a Rust dev environment?

With a "normal" nix-shell it can happen that a crate is linked against a lib that gets cleared by the gc. I have no idea if there's a solution for this.

@zimbatm
Copy link
Member

zimbatm commented Dec 13, 2021

Oops, I just realized that PRs have stacked up. Is this PR still relevant?

@happysalada
Copy link
Contributor

This PR is still relevant. Compilation on a rust project actually fails at the moment.
I tried to set up the environment variables in this PR however and it still failed.
There is something done in the hooks that makes darwin compilation work.
I should be able to get a reproducible example for anyone who has darwin. (probably this weekend)

@happysalada
Copy link
Contributor

alright, I tested on x86_darwin and couldn't reproduce the problem on a public repo.
On a private repo on aarch64-darwin. I had to
set RUSTFLAGS="-L${pkgs.darwin.apple_sdk.frameworks.CoreFoundation}/Library/Frameworks"
(also had to add the Security and SystemConfiguration)
I also ran into trouble compiling curl-sys library, for that I had to set
CFLAGS="-F ${pkgs.darwin.apple_sdk.frameworks.SystemConfiguration}/Library/Frameworks -I${pkgs.darwin.apple_sdk.libs.libDER}/include"

it took me quite a while to figure those out. I don't know exactly in which hook those are getting set. Inside the rustflags variable are stuff for the linker. So those could have been set into a wrapper for ld. The cflags could have been set in another wrapper. Not sure exactly which wrapper those are.

If anyone has an idea, I'm happy to test things.

@zimbatm
Copy link
Member

zimbatm commented Mar 20, 2022

Thanks for taking the time and test things. Are there any lessons to integrate back into the module?

@happysalada
Copy link
Contributor

I did some testing on linux and had to add glibc to the rustflags (just mentioning).

I quite like the api of this module, however I don't think I would implement it in this way. Let me try to work on this next week and make a PR. We can compare both approaches and discuss what to include and what not to include in the final solution.

basically I would use RUSTFLAGS, CFLAGS, and PKG_CONFIG_PATH which are all the environment variables I needed.

@happysalada
Copy link
Contributor

I just hit some linkers error on ubuntu, I see why those LD variables are important.
I had to add librdkafka to the LD_LIBRARY_PATH.
After that it complained about

/lib/x86_64-linux-gnu/libpthread.so.0: version `GLIBC_PRIVATE` not found (required by _nix_path_glibc-2-33-177/lib/librt.so.1)

I tried adding ${pkgs.glibc.dev}/include into the LD_INCLUDE_PATH but to no avail. I'm not sure how to fix this one. If anyone has an idea, I'm interested to try.

(this is just for ubuntu, for darwin, the previous fixes where enough).

@happysalada
Copy link
Contributor

so here is how I understand the situation.
If I add in the rustflags "${pkg.glibc}/lib" then rust compiles against that version of glibc. (I get a compiler error without that variable. However If I add this library to my LD_LIBRARY_PATH then at runtime, the system will need to find that library. I think it's usually done through the RPATH. in nixpkgs, ld has a wrapper to set the rpath. If we link against a library that the system doesn't have by default in it's RPATH (like on ubuntu any nix libs won't be included by default), we need to have a way to set the RPATH.
The previous error I get is the system's library trying to look for nix glibc and failing (I think).
We might have to keep the hooks that govern LD.

@happysalada
Copy link
Contributor

I'm not sure what to do next to tackle the linker problem.
I've found a way to make it work on darwin, but I'm sure it's not how nix traditionally does it (I've verified in the normal shell, the rustflags variable is not set).

@happysalada
Copy link
Contributor

I have some small updates on this.
using lld_13 and RUSTFLAGS="-C linkg-arg=-fuse-ld=lld" solved all the linker issues I had on darwin.

On ubuntu I still run into a glibc compilation error no matter what I do.
if anyone ever finds a solution to that one, I'm interested.

@happysalada
Copy link
Contributor

alright, I've got more news.

what is needed here is the following

  • LIBRARY_PATH set to $DEVSHELL_DIR/lib
  • RUSTFLAGS set to -F framework=$DEVSHELL_DIR/Library/Frameworks. The problem with that is that someone needs to be able to set rustflags if they want to. Would doing something with an eval like `eval = "-F framework=$DEVSHELL_DIR/Library/Frameworks:$RUSTFLAGS" work ? (RUSTDOCFLAGS need to be set to the same value)
  • CFLAGS set to -I $DEVSHELL_DIR/include -iframework $DEVSHELL_DIR/Library/Frameworks. Same here someone might want to set that value, so wondering if the trick with eval would work.

if that would work, I'm happy to make a PR.

PS: the reason why it was working before is that it was using the clang already installed on my computer. As soon as I had clang_13 in my shell everything started breaking down without the above fixes).

The other problem I had was that I had to bring some weird libs that I've never heard of before like darwin.apple_sdk.libs.libDER. This makes me thing that it's not the right way to solve this problem, but at least the issues are fixed.

@zimbatm
Copy link
Member

zimbatm commented Apr 2, 2022

Thanks for taking the time to untangle this mess.

Yeah, a PR where eval is used on RUSTFLAGS and CFLAGS sounds good.

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.

None yet

5 participants