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

Revert "rustc: add note about libiconv dependency" #213694

Merged
merged 1 commit into from
Feb 5, 2023

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Jan 31, 2023

Description of changes

This reverts commit edfbbaf.

I mistakingly believed that once 1.66.0 was used to bootstrap, we'd be
able to remove libiconv from rustc's build-time dependency tree on Darwin.
Sadly, this isn't the case, because src/tools/bootstrap depends on libc.

Additionally, it seems that my assessment in b1834a4
was wrong -- any dependency on libc will cause a requirement on
libiconv, due to rustc unconditionally linking every library specified
in link directives, no matter if the function is actually used.

This was worked around somewhat in rust-lang/libc#2944
by not linking libiconv if libc is only a dependency of std, but this
doesn't apply when libc is a dependency of anything else.

Maybe one day we'll just rip out libiconv from libc entirely (or hide it
behind a feature flag), but for now, we can just keep it in buildRustPackage's
buildInputs by default.

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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

This reverts commit edfbbaf.

I mistakingly believed that once 1.66.0 was used to bootstrap, we'd be
able to remove libiconv from rustc's build-time dependency tree on Darwin.
Sadly, this isn't the case, because src/tools/bootstrap depends on libc.

Additionally, it seems that my assessment in b1834a4
was wrong -- *any* dependency on `libc` will cause a requirement on
libiconv, due to rustc unconditionally linking every library specified
in `link` directives, no matter if the function is actually used.

This was worked around somewhat in rust-lang/libc#2944
by not linking libiconv if libc is only a dependency of std, but this
doesn't apply when `libc` is a dependency of anything else.

Maybe one day we'll just rip out libiconv from `libc` entirely (or hide it
behind a feature flag), but for now, we can just keep it in `buildRustPackage`'s
`buildInputs` by default.
@winterqt
Copy link
Member Author

I'd appreciate proof reading of the commit message -- I just noticed a few mistakes. Thanks!

@zowoq
Copy link
Contributor

zowoq commented Jan 31, 2023

/mistakingly/mistakenly/

@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 Jan 31, 2023
@figsoda
Copy link
Member

figsoda commented Feb 1, 2023

this can now target staging-next: #214010, even though it probably doesn't matter

@mweinelt mweinelt added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 5, 2023
@vcunat vcunat changed the base branch from staging to staging-next February 5, 2023 14:37
@vcunat vcunat merged commit 52bef35 into NixOS:staging-next Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants