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

[darwin] rustPlatform.buildRustPackage: do not add libiconv to buildInputs #122231

Closed

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented May 8, 2021

Motivation for this change

Many buildRustPackage-based derivations require libiconv as a build
input on Darwin. This is caused by libc unconditionally linking
against libiconv. However, this was recently changed:

rust-lang/libc#2150

We will probably see this dependency disappear over the coming months
once a new libc crate version is released and other crates adopt
this new version.

Since this is already an optional dependency and it will disappear
in the coming time, we should probably not unconditionally add it
to all Rust crates.

Can't test, because I don't have a Darwin machine.

cc @jonringer

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Many buildRustPackage-based derivations require libiconv as a build
input on Darwin. This is caused by `libc` unconditionally linking
against libiconv. However, this was recently changed:

rust-lang/libc#2150

We will probably see this dependency disappear over the coming months
once a new `libc` crate version is released and other crates adopt
this new version.

Since this is already an optional dependency and it will disappear
in the coming time, we should probably not unconditionally add it
to all Rust crates.
@danieldk danieldk requested review from andir and zowoq as code owners May 8, 2021 19:04
@danieldk danieldk mentioned this pull request May 8, 2021
10 tasks
@prusnak
Copy link
Member

prusnak commented May 8, 2021

libiconv needs to be reintroduced to pkgs/development/compilers/rust/cargo.nix again, because 252bf94 removed it

@prusnak
Copy link
Member

prusnak commented May 8, 2021

Shall we switch this to draft so it's not accidentally merged before libc crate is updated to 0.2.95 which fixes the iconv issue?

@ofborg ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 8, 2021
@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 8, 2021

I can try testing this on darwin -- do you have a particular subset set of packages to test that you know are relevant? FWIW I've been using https://github.com/jtojnar/nixpkgs-fastmac, in addition to the mac in my closet. It seems to work pretty well.

@danieldk
Copy link
Contributor Author

danieldk commented May 8, 2021

Shall we switch this to draft so it's not accidentally merged before libc crate is updated to 0.2.95 which fixes the iconv issue?

But we can’t switch wholesale to 0.2.95 once it is out, we have to rely on the various upstream crates to update their lock files.

It’s really a dependency that should be tracked per derivation.

But I am fine with making this a draft. Then we have this workaround for the 21.05 release and can remove it later.

edit: seems like cannot make this a draft on my phone. Feel free to convert it!

@jonringer
Copy link
Contributor

thanks for starting a new PR :)

@jonringer jonringer added the 6.topic: darwin Running or building packages on Darwin label May 8, 2021
@jonringer jonringer changed the title rustPlatform.buildRustPackage: do not add libiconv to buildInputs [darwin] rustPlatform.buildRustPackage: do not add libiconv to buildInputs May 8, 2021
@prusnak prusnak marked this pull request as draft May 8, 2021 21:50
@danieldk danieldk marked this pull request as ready for review June 8, 2021 08:07
@danieldk
Copy link
Contributor Author

danieldk commented Jun 8, 2021

0.2.95 was released two weeks ago, so taking this PR out of draft.

https://github.com/rust-lang/libc/releases/tag/0.2.95

This is the first version that does not unconditionally link libiconv on darwin.

Of course, downstream crates have to update to this version of the libc crate, but the number of crates requiring iconv on darwin should shrink drastically over time. For those that need it, we should have this dependency in their buildInputs, rather than having this in the buildInputs of every buildRustPackage derivation.

@Profpatsch
Copy link
Member

rustPackages is currently broken on 21.05 because libiconv was removed. Can we please introduce it again?

@danieldk
Copy link
Contributor Author

rustPackages is currently broken on 21.05 because libiconv was removed. Can we please introduce it again?

Seems to be there in 21.05?

++ lib.optionals stdenv.hostPlatform.isDarwin [ libiconv ]

@Profpatsch
Copy link
Member

Seems to be there in 21.05?

lorri still breaks as far as I can see: https://github.com/nix-community/lorri/pull/71/checks?check_run_id=3812404819

@danieldk
Copy link
Contributor Author

danieldk commented Oct 6, 2021

Seems to be there in 21.05?

lorri still breaks as far as I can see: https://github.com/nix-community/lorri/pull/71/checks?check_run_id=3812404819

From a quick look, lorri seems to use Carnix? Carnix doesn't use buildRustPackage, but buildRusetCrate, so an override needs to be added to

https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/rust/default-crate-overrides.nix

for the libc crate.

@Profpatsch
Copy link
Member

From a quick look, lorri seems to use Carnix? Carnix doesn't use buildRustPackage, but buildRusetCrate, so an override needs to be added to

The PR I linked actually switches it over to crate2nix since carnix is super out of date at this point and failed with the newer lock file format.

@danieldk
Copy link
Contributor Author

From a quick look, lorri seems to use Carnix? Carnix doesn't use buildRustPackage, but buildRusetCrate, so an override needs to be added to

The PR I linked actually switches it over to crate2nix since carnix is super out of date at this point and failed with the newer lock file format.

crate2nix also uses buildRustCrate.

@Profpatsch
Copy link
Member

tbh I stopped caring about Darwin and call upon @NixOS/darwin-maintainers

@Profpatsch
Copy link
Member

I mean this has been broken since May or even earlier, and is exactly why NixOS/rfcs#112 exists.

@domenkozar
Copy link
Member

@Profpatsch I'd kindly ask you to be less hateful and show some empathy for others.

@winterqt
Copy link
Member

winterqt commented Nov 12, 2021

This is still required. I tried to figure out why this happens a few months ago, but I only got a little info (it's an upstream issue, to my knowledge). I'll try to pick it back up though.

@winterqt
Copy link
Member

(and by "this" I mean that libiconv still needs to be present)

@uri-canva
Copy link
Contributor

For anyone else having issues with buildRustCrate, crate2nix or carnix and ending up here, the fix is in nixpkgs master: c9f0c6f.

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 14, 2021
@Profpatsch
Copy link
Member

@Profpatsch I'd kindly ask you to be less hateful and show some empathy for others.

apathy ≠ hate

@danieldk danieldk closed this Jan 16, 2022
@zowoq zowoq mentioned this pull request Sep 28, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: darwin Running or building packages on Darwin 6.topic: rust 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 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.

9 participants