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.locale: restore locale data #347817

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Conversation

reckenrode
Copy link
Contributor

This is a follow up to #346043 to add the missing locale data. It’s not exactly the same as the data used on macOS, but it’s close. This is being done separately in case the PR is rejected.

Replacement for #347642.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Oct 10, 2024
@reckenrode reckenrode marked this pull request as ready for review October 10, 2024 23:46
@nix-owners nix-owners bot requested a review from philiptaron October 10, 2024 23:47
@emilazy emilazy deleted the branch NixOS:staging October 11, 2024 00:01
@emilazy emilazy closed this Oct 11, 2024
@emilazy emilazy reopened this Oct 11, 2024
@emilazy emilazy changed the base branch from reckenrode/darwin-sdk-refactor to staging October 11, 2024 00:06
@emilazy
Copy link
Member

emilazy commented Oct 11, 2024

I will merge this in a bit (trying to avoid a stdenv rebootstrap right now).

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild labels Oct 11, 2024
@ofborg ofborg bot requested a review from toonn October 11, 2024 05:13
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 11, 2024
Comment on lines 99 to 101
# Update `bsdmake` references to `bmake`
substituteInPlace "$LOCALE_TOPLEVEL/Makefile" \
--replace-fail bsdmake bmake
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don’t have the exact bsdmake Apple expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don’t because I deleted it and replaced it with bmake, which is the portable BSD make from NetBSD.

Comment on lines 128 to 131
buildPhase =
''
ninjaBuildPhase
''
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the preBuild/postBuild hooks. Is there a way to just put the bmake stuff in postBuild, or maybe we can make the hooks compose better somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ninjaBuildPhase and bmakeBuildPhase run the hooks. Putting bmakeBuildPhase in postBuild causes it to go into an infinite loop. I want the flag handling of both hooks without their invoking preBuild and postBuild hooks, but there’s no way to do that currently.

Comment on lines 133 to 143
# Cross-compilation requires using adv_cmds from the buildPlatform.
(
if stdenvNoCC.buildPlatform.canExecute stdenvNoCC.hostPlatform then
''
export PATH=$PWD:$PATH
''
else
''
export PATH=$PWD:${lib.getBin buildPackages.darwin.adv_cmds}/bin
''
)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, why don’t we just split this locale stuff back into its own package? This clearly just wants a nativeBuildInputs = [ darwin.adv_cmds ];, we’re having to compose two separate build systems in one derivation, and one of them goes to a different output anyway. I think it would be a lot nicer to just have a separate package for the locale data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s not unusual for a package that needs to generate some data to have special handling for cross-compilation (e.g., ICU and ncurses). I can move it to a passthru derivation, so adv_cmds.locale works as it did before.

@reckenrode reckenrode force-pushed the push-kuqklzxopwyr branch 3 times, most recently from 697e899 to 7354aeb Compare October 11, 2024 21:36
The locale data used on macOS has not been included in a source release
since adv_cmds-118. Fortunately, that data can be parsed by the current
version of adv_cmds. It’s a bit old, but other sources of data (such as
FreeBSD) are not compatible enough and may cause divergent behavior.
@reckenrode reckenrode changed the title darwin.adv_cmds: restore locale data darwin.locale: restore locale data Oct 11, 2024
@emilazy emilazy merged commit f63d998 into NixOS:staging Oct 11, 2024
10 of 11 checks passed
@reckenrode reckenrode deleted the push-kuqklzxopwyr branch October 11, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants