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

rustc: propagate libiconv on darwin #190093

Merged
merged 1 commit into from Sep 28, 2022

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Sep 7, 2022

Description of changes

Rust binaries are unconditionally linked to libiconv on Darwin (see rust-lang/libc#2870). We already add it as a dependency in buildRustPackage, so let's go a step further and propagate it.

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/)
  • 22.11 Release Notes (or backporting 22.05 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.

Rust binaries are unconditionally linked to libiconv on Darwin (see rust-lang/libc#2870). We already add it as a dependency in `buildRustPackage`, so let's go a step further and propagate it.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Sep 7, 2022
@zowoq
Copy link
Contributor

zowoq commented Sep 7, 2022

@ofborg eval

@winterqt
Copy link
Member Author

winterqt commented Sep 7, 2022

Why do you think retriggering eval will fix it, @zowoq? Nothing Boost-related (which is why it was failing previously) was merged into staging since the failed eval. I think it'll just fail again, unless I'm missing something.

@zowoq
Copy link
Contributor

zowoq commented Sep 7, 2022

Sometimes it is an eval/build failure, sometimes it just chokes .. can usually guess by looking at other recent staging PRs.

Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

Guess we don't really have a choice here.

Are you planning to remove the unnecessary libiconv from packages after this PR is in master?

@winterqt
Copy link
Member Author

Are you planning to remove the unnecessary libiconv from packages after this PR is in master?

Yes, shouldn't be too many (since most packages use buildRustPackage, I'll only have to touch packages that use the setup hooks and support Darwin, so I don't suspect a lot).

Might be better to just do it in this PR? Do you suggest not doing so only because we can get feedback from OfBorg, or...?

@zowoq
Copy link
Contributor

zowoq commented Sep 19, 2022

shouldn't be too many

Quick grep looks like 100 or so?

Might be better to just do it in this PR? Do you suggest not doing so only because we can get feedback from OfBorg, or...?

Not doing it as part of this PR would mean we avoid any conflicts while it going though staging/staging-next.

@winterqt
Copy link
Member Author

Not doing it as part of this PR would mean we avoid any conflicts while it going though staging/staging-next.

What do you mean by avoid? We can always just watch the staging-next eval... maybe the argument can be made that it would be more annoying, but it's not that bad.

@zowoq
Copy link
Contributor

zowoq commented Sep 19, 2022

What do you mean by avoid?

Avoiding git conflicts in the master->staging-next->staging merges if someone modifies the same file(s) on master.

@winterqt
Copy link
Member Author

winterqt commented Sep 19, 2022

Avoiding git conflicts in the master->staging-next->staging merges if someone modifies the same file(s) on master.

I've done treewide changes in staging before, and have never ran into any conflicts. (I have no clue how the merge action would even handle that, if at all, tbh.)

On second thought, the treewide change I'm thinking about had to be done in staging, while this doesn't have to be. So I'm comparing apples and oranges I guess.

To be clear, I was just trying to understand why doing it in master is better -- I now understand that you meant git conflicts. Happy to do it in a separate PR for that reason.

@vcunat vcunat merged commit 292756e into NixOS:staging Sep 28, 2022
@winterqt winterqt deleted the rustc-propagate-libiconv branch September 28, 2022 13:15
@winterqt
Copy link
Member Author

FWIW any of the instances that can be removed (if any) could have been removed over a year ago. I'll look at it eventually and make a separate though; doubt it has to go through staging.

@zowoq
Copy link
Contributor

zowoq commented Sep 28, 2022

FWIW any of the instances that can be removed (if any) could have been removed over a year ago.

While something like #122231 was a possibility I hadn't been to worried about doing any removals but now that it isn't going to happen may as well do a clean up.

If I have some free time I may do it myself, if I'm going to start working on it I'll leave a comment here so we aren't duplicating work.

@tjni
Copy link
Contributor

tjni commented Nov 14, 2022

Found this in the pending Rust 1.66 release notes interesting:

I don't know if this impacts what we choose to do here.

@winterqt
Copy link
Member Author

It does! In fact, my issue made it happen :)

@tjni If you end up doing the 1.66 bump, I'd appreciate a poke about this then. Thanks!

@riking
Copy link

riking commented Dec 13, 2022

1.66 release is this week.

@tjni tjni mentioned this pull request Dec 15, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants