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

nixos/i18n: don't build all supportedLocales by default #177318

Merged
merged 1 commit into from
Jun 25, 2022

Conversation

SuperSandro2000
Copy link
Member

Description of changes
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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 11, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 11, 2022
@vcunat vcunat added the 6.topic: closure size The final size of a derivation, including its dependencies label Jun 12, 2022
@dasJ dasJ requested a review from ajs124 June 21, 2022 10:50
@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 23, 2022
@vcunat vcunat merged commit 24b4356 into NixOS:master Jun 25, 2022
@SuperSandro2000 SuperSandro2000 deleted the i18n.supportedLocales branch June 25, 2022 20:33
@gebner
Copy link
Member

gebner commented Jun 26, 2022

This broke any custom extraLocaleSettings (like LC_PAPER or LC_TIME etc.).

@gravndal
Copy link
Contributor

Shouldn't this have been announced here: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574?

Either way, wouldn't it make more sense to default to something like:

    supportedLocales = with config.i18n; lib.unique (
      (lib.mapAttrsToList (n: v: v + "/UTF-8")
        (lib.filterAttrs (n: v: n != "LANGUAGE") extraLocaleSettings)
      ) ++ [ (defaultLocale + "/UTF-8") ]
    );

@SuperSandro2000
Copy link
Member Author

Shouldn't this have been announced here: discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574?

I didn't even know that thread existed.

Either way, wouldn't it make more sense to default to something like:

Please do a PR with that.

This broke any custom extraLocaleSettings (like LC_PAPER or LC_TIME etc.).

Presumable only when they are set to something other than your display language?

@gravndal
Copy link
Contributor

#179194

@gebner
Copy link
Member

gebner commented Jun 26, 2022

This broke any custom extraLocaleSettings (like LC_PAPER or LC_TIME etc.).

Presumable only when they are set to something other than your display language?

That's their only use, yes. They default to the display language, so you don't need the override if it's the same.

@Artturin
Copy link
Member

Artturin commented Jul 2, 2022

#179607

@Artturin
Copy link
Member

Artturin commented Jul 5, 2022

#179486

@sternenseemann
Copy link
Member

I think unconditionally generating C.UTF-8/UTF-8 is probably also a good idea.

@Profpatsch
Copy link
Member

Either way, wouldn't it make more sense to default to something like:

Please do a PR with that.

No, that’s not how that works.

You can’t break other people’s systems and then tell them to fix it themselves.

@Profpatsch
Copy link
Member

So we should revert this and reevaluate.

@Profpatsch
Copy link
Member

Like, for example, not generating C.UTF-8 by default shows that there is a fundamental gap in understanding here.

@SuperSandro2000
Copy link
Member Author

No, that’s not how that works.

If someone is presenting me a working code solution to a problem I think it is very reasonable to ask them to create a PR with that. The other option I would have would be to commit it as my own and then ask the same person to test their own code which is a bit weird.

You can’t break other people’s systems and then tell them to fix it themselves.

That is wrong. Every change has the potential to break something somewhere in some combination of the infinity of potential system combinations. We can only consider so much when doing a change. That's why I wrote a changelog entry because I knew before that if people relied silently on other locales their setup would break. There is just no other way to not break this. I and two reviewers didn't think about extraLocaleSettings or eo and only almost two weeks later we noticed that some things break.

Also my time is not infinite and between real life and other work and projects I might only find time to do this in a few days in which the fix could have been already be commited, merged and be on unstable.

So we should revert this and reevaluate.

Instead of hastily reverting changes and later reverting the revert, we should first consider if a small addition/change of code fixes those use cases or people need to adopt their config and explicitly mention a locale.

Like, for example, not generating C.UTF-8 by default shows that there is a fundamental gap in understanding here.

First of all, I know how locales work and the special meaning behind c.utf-8.

Then this must have been broken for a good amount of time in the minimal profile which is used in containers similar to environment.noXlibs.


Now back to fixing this instead of doing discussions.

First of all a big thanks to @gravndal for the two PRs fixing the situation when more complex settings are used. I'll really appreciate it.

Also regarding to #179486 : This must have been always broken but since we generated all locales by default this was never noticed. So to say we unconvered more bugs with this change that where always there.

Then why I didn't notice this while testing: I had C.UTF-8 set in i18n.supportedLocales so I couldn't. Happens when your configuration is multiple thousand lines long. Going to fix that in a PR right now.

@Profpatsch
Copy link
Member

That is wrong. Every change has the potential to break something somewhere in some combination of the infinity of potential system combinations.

Given that this PR has neither any description, nor a commit description, I would not assume much thought has been put into possible breakage.

@dasJ
Copy link
Member

dasJ commented Jul 7, 2022

It's called "unstable" for a reason…

@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Jul 7, 2022

I wanted to write that, too. 😂

Yes, breakages can happen even if we all try our best to avoid them.


Given that this PR has neither any description, nor a commit description

yeah, I should do them more often but usually I just skip them because I don't read them that often when reviewing either.


C.UTF-8 fixup is in #180513

sternenseemann added a commit to openlab-aux/vuizvui that referenced this pull request Jul 8, 2022
See NixOS/nixpkgs#177318 for the lovely change
that causes your locales to disappear.
@Artturin
Copy link
Member

#180935

@SuperSandro2000
Copy link
Member Author

#180935

Could be #180368

@JonathanReeve
Copy link
Contributor

If this is what's causing #179607, please revert this. It's causing my system to fail to build in my language, and everyone else who also uses Nixos in that language.

@SuperSandro2000
Copy link
Member Author

I don't think that is good way to move this forward. I'd rather add an exception for that language instead of reverting improvements which where much needed and everyone benefits of.

@Artturin
Copy link
Member

Another issue #183960

@gravndal
Copy link
Contributor

gravndal commented Jul 30, 2022

@Artturin I don't think that's actually caused by this.

From some limited testing, it seems that if your current configured locales don't match the new supported locales reliably triggers those warnings.

A situation likes this can naturally occur if you're removing a locale from your extraLocaleSettings, e.g. I can trigger it by removing my LC_TIME=en_DK.UTF-8 (the only location where I reference this specific locale), and then do a rebuild switch.

This happens (I think) because my current shell sessions still exports the, now old, LC_TIME.

Edit: though, actually, that's just the warnings, not the error...
Edit2+: yeah, that's probably just a misconfigured locale, en_AT.UTF-8 isn't included with glibc.

Also:

 - channels(tmn): `"nixos-22.05-22.05.1806.e732e1fdbf7"`
 - channels(root): `"nixos-21.11.337967.573603b7fdb"`

This wasn't ever backported was it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: closure size The final size of a derivation, including its dependencies 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 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.

10 participants