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

Rework system locale #1423

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

codefiles
Copy link
Contributor

@codefiles codefiles commented Aug 14, 2022

PR Description:

  • Update list_locales() to use /usr/share/i18n/SUPPORTED.
  • Verify locales are supported by checking against /usr/share/i18n/SUPPORTED.
  • Uncomment entries in /etc/locale.gen or append if an entry is not found to uncomment.
  • Use localedef for generating locales instead of locale-gen.
  • Do not write to /etc/locale.gen when it does not need to be modified.
  • Do not generate locales when the given locales are already generated.
  • Do not set the system locale when it is already set to the given locale.
  • Strip encodings from options in the guided installer Locale language menu leaving only languages since encodings are selected in the Locale encoding menu.
  • Add support for generating multiple locales (to add to the guided installer in the future).

Tests and Checks

  • I have tested the code!

@codefiles codefiles requested a review from Torxed as a code owner August 14, 2022 00:52
@codefiles codefiles force-pushed the rework-locale branch 6 times, most recently from 6088114 to 7f668da Compare August 14, 2022 05:08
@codefiles codefiles marked this pull request as draft August 14, 2022 06:01
@Torxed
Copy link
Member

Torxed commented Aug 14, 2022

This looks great! It's a breaking change as it will break all existing configurations (which is fine), so I'd like to propose that we postpone merging this (when you feel ready with the PR) until next release, that way we can add a warning to the current functionalities that they're being deprecated.

@svartkanin
Copy link
Collaborator

svartkanin commented Aug 14, 2022

In previous changes like this we did support backwards compatibility, for example superuser configuration.
I think it shouldn't be too difficult to get that in here, essentially when parsing the config we need to check if there's an old version entry present and if so convert it to the new one.

Something like in archinstall/lib/models/users.py

However, i am very much in favor of providing a deprecated warning (also introduced in users) to inform that this will go away. This way we'll clean up this code and don't have to maintain it.

@Torxed
Copy link
Member

Torxed commented Aug 14, 2022

However, i am very much in favor of providing a deprecated warning (also introduced in users) to inform that this will go away. This way we'll clean up this code and don't have to maintain it.

Yea this would be optimal, since we have about 1-2 months between releases any way I think having a deprecation warning is more than enough. We could do the same for the superuser configuration tbh.

@codefiles
Copy link
Contributor Author

I've changed the scope of the pull request to focus on non-breaking changes. I will follow it up with a pull request for the unification of locale language and locale encoding into locale if that is a desired change.

@codefiles codefiles marked this pull request as ready for review August 17, 2022 00:27
@codefiles codefiles force-pushed the rework-locale branch 5 times, most recently from 42f1510 to 86f76dc Compare August 17, 2022 07:46
@Torxed
Copy link
Member

Torxed commented Aug 17, 2022

localedef was a new one for me. Looks really good. I'll try to run a few tests in a day or so and merge this in :)

@codefiles codefiles force-pushed the rework-locale branch 7 times, most recently from e488c34 to f0b2921 Compare August 18, 2022 03:18
@codefiles
Copy link
Contributor Author

codefiles commented Aug 18, 2022

@Torxed, considering what you mentioned here: #1200 (comment), is there a reason locale and hostname are not parameters to minimal_installation()?

-def minimal_installation(self, testing=False, multilib=False) -> bool:
+def minimal_installation(self, testing :bool = False, multilib :bool = False, hostname :str = 'archinstall', locale :Tuple[str, str] = ('en_US', 'UTF-8')) -> bool:

@codefiles
Copy link
Contributor Author

codefiles commented Sep 5, 2022

Not the right place for this but these Arch Linux glibc package changes might be improvements:

  • Provide localedata/SUPPORTED as /usr/share/i18n/SUPPORTED. Though the contents of the glibc localedata/SUPPORTED file is used to create /etc/locale.gen, it would be beneficial if it was also supplied separately as /usr/share/i18n/SUPPORTED since the contents of /etc/locale.gen can be overwritten as it is intended to be edited. It appears this file is being distributed in this manner with the glibc packages in other distributions such as Debian and Gentoo.
  • Remove # en_US.UTF-8 UTF-8 line from examples in locale.gen.txt since that line, instead of the later #en_US.UTF-8 UTF-8, can become uncommented in /etc/locale.gen with the use of localectl set-locale en_US.UTF-8 or remove all examples since they are superfluous.

@codefiles
Copy link
Contributor Author

codefiles commented Sep 5, 2022

Here is what this pull request looks like working with multiple locales:

>>> from archinstall.lib.locale_helpers import Locale, LocaleUtils
>>> locale_names = ['en_US.UTF-8', 'de_DE.UTF-8', 'de_DE.ISO-8859-1', 'de_DE.ISO-8859-15@euro']
>>> locales = []
>>> for locale_name in locale_names:
...     locales.append(Locale(locale_name))
...
>>> LocaleUtils(locales).run()
Uncommented entries in locale-gen configuration file
  de_DE.UTF-8 UTF-8
  de_DE ISO-8859-1
  de_DE@euro ISO-8859-15
  en_US.UTF-8 UTF-8
Generating locales...
  de_DE.UTF-8...
  de_DE.ISO-8859-1...
  de_DE.ISO-8859-15@euro...
  en_US.UTF-8...
Generation complete.
System locale set to en_US.UTF-8
True

This is ready to be reviewed and tested. More to come in future pull requests as mentioned here #1435 (comment).

@Torxed
Copy link
Member

Torxed commented Sep 5, 2022

Great work on this, I'll pass along the message about glibc and the examples file.
I'll test this most likely next week, as this week is a bit busy and scheduled.

@codefiles
Copy link
Contributor Author

I appreciate you passing along the message about the Arch Linux glibc package. Would you please link to the comment when passing the information along? I have just edited that comment for additional clarity.

archlinux-github pushed a commit to archlinux/svntogit-packages that referenced this pull request Sep 6, 2022
Add SUPPORTED file to package as done by other distributions in order to provide
the set of available locales in a unmodified file.
Also remove examples in locale.gen to avoid matches of tools as sed and grep.

Credit @ archlinux/archinstall#1423 (comment)


git-svn-id: file:///srv/repos/svn-packages/svn@455119 eb2447ed-0c53-47e4-bac8-5bc4a241df78
archlinux-github pushed a commit to archlinux/svntogit-packages that referenced this pull request Sep 6, 2022
Add SUPPORTED file to package as done by other distributions in order to provide
the set of available locales in a unmodified file.
Also remove examples in locale.gen to avoid matches of tools as sed and grep.

Credit @ archlinux/archinstall#1423 (comment)

git-svn-id: file:///srv/repos/svn-packages/svn@455119 eb2447ed-0c53-47e4-bac8-5bc4a241df78
@Torxed
Copy link
Member

Torxed commented Sep 6, 2022

@codefiles As per suggested, your change is now merged upstream (archlinux/svntogit-packages@3c537be). Great work!

@codefiles
Copy link
Contributor Author

codefiles commented Sep 7, 2022

Another message regarding the Arch Linux glibc package:

Note
The patch file supported.diff implements everything below.

  • Debian [0][1] and Gentoo [2] process localedata/SUPPORTED to generate /usr/share/i18n/SUPPORTED. Use the following sed command on localedata/SUPPORTED to generate /usr/share/i18n/SUPPORTED.
sed -e '1,3d' -e 's|/| |g' -e 's| \\||g'
  • Modify the line with the sed command for /etc/locale.gen to remove trailing whitespace,
-  sed -e '1,3d' -e 's|/| |g' -e 's|\\| |g' -e 's|^|#|g' \
+  sed -e '1,3d' -e 's|/| |g' -e 's| \\||g' -e 's|^|#|g' \

or use the following sed command on the generated /usr/share/i18n/SUPPORTED rather than localedata/SUPPORTED.

sed 's|^|#|g'
  • Consider adding a note about /usr/share/i18n/SUPPORTED to locale.gen.txt.
-#  A list of supported locales is included in this file.
-#  Uncomment the ones you need.
+#  A list of supported locales is given in /usr/share/i18n/SUPPORTED
+#  and is included in this file. Uncomment the needed locales below.

[0] https://salsa.debian.org/glibc-team/glibc/-/blob/1ad03bfd35cb0258dc1e73c1aab4b9bc128a3ea2/debian/rules.d/build.mk#L240-244
[1] https://salsa.debian.org/glibc-team/glibc/-/blob/1ad03bfd35cb0258dc1e73c1aab4b9bc128a3ea2/debian/generate-supported.mk
[2] https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-libs/glibc/glibc-9999.ebuild?id=14ae8ab7798898eb196ddecd9c58ff1f3e8ba306#n1401

@Torxed
Copy link
Member

Torxed commented Sep 7, 2022

sed -e '1,3d' -e 's|/| |g' -e 's| \\||g'

Isn't this a bit of a workaround for an upstream glibc issue tho? Or did I misunderstood why we remove trailing whitespaces?

@codefiles
Copy link
Contributor Author

codefiles commented Sep 7, 2022

sed -e '1,3d' -e 's|/| |g' -e 's| \\||g'

Isn't this a bit of a workaround for an upstream glibc issue tho? Or did I misunderstood why we remove trailing whitespaces?

You are misunderstanding why I'm requesting trailing whitespace to be removed. In the glibc PKGBUILD a sed command is being run on localedata/SUPPORTED to generate the /etc/locale.gen file. The command in the PKGBUILD is sed -e '1,3d' -e 's|/| |g' -e 's|\\| |g' -e 's|^|#|g' and the -e 's|\\| |g' portion replaces the backslash at the end of a line containing a locale such as aa_DJ.UTF-8/UTF-8 \ with a space. Changing the -e 's|\\| |g' portion to -e 's| \\||g' removes the space at the end before the backslash and does not add a space.

sed 's|\\| |g'

-aa_DJ.UTF-8/UTF-8 \
+aa_DJ.UTF-8/UTF-8  

sed 's| \\||g'

-aa_DJ.UTF-8/UTF-8 \
+aa_DJ.UTF-8/UTF-8

The following demonstrates that it makes a difference:

$ cat localedata/SUPPORTED
# This file names the currently supported and somewhat tested locales.
# If you have any additions please file a glibc bug report.
SUPPORTED-LOCALES=\
aa_DJ.UTF-8/UTF-8 \
aa_DJ/ISO-8859-1 \
aa_ER/UTF-8 \
aa_ER@saaho/UTF-8 \
...
$ sed -e '1,3d' -e 's|/| |g' -e 's|\\| |g' localedata/SUPPORTED
aa_DJ.UTF-8 UTF-8
aa_DJ ISO-8859-1
aa_ER UTF-8
aa_ER@saaho UTF-8
...
$ sed -e '1,3d' -e 's|/| |g' -e 's| \\||g' localedata/SUPPORTED
aa_DJ.UTF-8 UTF-8
aa_DJ ISO-8859-1
aa_ER UTF-8
aa_ER@saaho UTF-8
...
$ diff <(!-2) <(!!)
diff -qs <(sed -e '1,3d' -e 's|/| |g' -e 's|\\| |g' localedata/SUPPORTED) <(sed -e '1,3d' -e 's|/| |g' -e 's| \\||g' localedata/SUPPORTED)
Files /dev/fd/63 and /dev/fd/62 differ

The /usr/share/i18n/SUPPORTED file can be generated then sed 's|^|#|g' can be run on it to generate the /etc/locale.gen, just adding the processed SUPPORTED file as commented lines to the /etc/locale.gen file.

@codefiles
Copy link
Contributor Author

codefiles commented Sep 7, 2022

This may also clear things up:

$ curl -sO "https://salsa.debian.org/glibc-team/glibc/-/raw/sid/debian/generate-supported.mk"
$ make -sf generate-supported.mk IN=localedata/SUPPORTED OUT=SUPPORTED.debian
$ sed \
	-e "/^#/d" \
	-e "/SUPPORTED-LOCALES=/d" \
	-e "s: \\\\::g" -e "s:/: :g" \
	localedata/SUPPORTED > SUPPORTED.gentoo
$ diff -qs SUPPORTED.debian SUPPORTED.gentoo
Files SUPPORTED.debian and SUPPORTED.gentoo are identical
$ diff -qs SUPPORTED.debian <(sed -e '1,3d' -e 's|/| |g' -e 's|\\| |g' localedata/SUPPORTED)
Files SUPPORTED.debian and /dev/fd/63 differ
$ diff -qs SUPPORTED.debian <(sed -e '1,3d' -e 's|/| |g' -e 's| \\||g' localedata/SUPPORTED)
Files SUPPORTED.debian and /dev/fd/63 are identical

@Torxed
Copy link
Member

Torxed commented Sep 7, 2022

I see, I'll pass it along! :) Apologies for not getting it straight away, I should maybe have spent a few more minutes reading through your initial example. I was in a bit of a rush :)

@codefiles
Copy link
Contributor Author

It is ok, should be a lot clearer now. It is hard to find a middle ground between being terse and verbose when describing issues. Terse descriptions might lack clarity and verbose descriptions risk being ignored.

@codefiles
Copy link
Contributor Author

codefiles commented Sep 22, 2022

In regards to the Arch Linux glibc package, two of the bullets in my earlier comment, #1423 (comment), have now been resolved by:

@freswa, the revised patch file supported.diff targets the second bullet from my earlier comment, #1423 (comment), that is not yet addressed. If implemented it would:

  • Eliminate the whitespace at the end of entries in /etc/locale.gen.
  • Deduplicate the sed commands that are working on the glibc file localedata/SUPPORTED to create /etc/locale.gen and /usr/share/i18n/SUPPORTED.
  • Reduce maintenance of the PKGBUILD should the glibc file localedata/SUPPORTED change in a way that required modification of the sed commands.

@codefiles
Copy link
Contributor Author

The latest commit requires the next release of the glibc package for archlinux/svntogit-packages@b09829c.

@Torxed
Copy link
Member

Torxed commented Sep 29, 2022

Perfect, I'll merge this as soon as https://archlinux.org/packages/core/x86_64/glibc/ is updated (the upstream commit is newer than latest package release)

@codefiles
Copy link
Contributor Author

The glibc package has been updated.

Be aware that if this is merged the current ISO (2022.10.01) will not meet the required version of glibc when using master.

@Torxed
Copy link
Member

Torxed commented Oct 6, 2022

Good heads up! Our ISO build and tests should still work as it always build the latest, but people tend to run the git version on the officiall ISO so, gut feeling says this will be the last PR into the next release, as close to the 1-11-2022 as possible, is that ok?

Copy link
Member

@Torxed Torxed left a comment

Choose a reason for hiding this comment

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

Changes looks promising, I understand I'm a bit late approving this. Will this interfere with the recent changes to the locale perhaps?

@codefiles
Copy link
Contributor Author

Let's not merge this yet. I feel that it is stale now so I am changing this to a draft till I review it again.

@codefiles codefiles marked this pull request as draft September 20, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants