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

WIP: gettext: refactor and split in multiple packages #79171

Draft
wants to merge 1 commit into
base: staging
from

Conversation

@lsix
Copy link
Member

@lsix lsix commented Feb 3, 2020

Motivation for this change

This PR solves #73288

Following upstream recommendations, gettext is split into 3 distinct
packages:

  • gettext-runtime
  • gettext-tools
  • libtextstyle

See https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=PACKAGING;h=f4bae4561ec70a9de2532502b5c2358bb976036c;hb=55b8fe9a2f12b22eb3ee6baf973e03e60b3a9915

Given that for each sub-package we use sourceRoot, the patches do not
apply directly when fetch from upstream git and have to be editted to
strip the top level dir.

For the time being, gettext is turned into an alias to
gettext-runtime to ease transition.

A tree-wide update could be tried to replace all occurances of a gettext dependency with gettext-runtime if required by review.

I am also very open to discussions on how I did implement the 3 packages that share 1 source tarball.

If we are too close to the last staging changes before release-20.03, I have no problem to postpone this MR (cc @FRidh ).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@FRidh FRidh added this to WIP in Staging via automation Feb 3, 2020
@FRidh FRidh added this to the 20.03 milestone Feb 3, 2020
@FRidh FRidh moved this from WIP to Needs review in Staging Feb 3, 2020
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch 2 times, most recently from 0ef0cd9 to 0b070e2 Feb 3, 2020
@ofborg ofborg bot requested review from lovek323, andir, np, vrthra, edolstra and zimbatm Feb 3, 2020
@disassembler disassembler modified the milestones: 20.03, 20.09 Feb 5, 2020
nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved
pkgs/development/tools/gettext-tools/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/gettext-tools/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/gettext-tools/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/gettext-tools/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/gettext-tools/default.nix Outdated Show resolved Hide resolved
@lsix
Copy link
Member Author

@lsix lsix commented Feb 6, 2020

@jtojnar thanks for the review

I am so ashamed of the number of of typos I did let through…

I am rebuilding everything to test at the moment. I’ll push when I have checked that it builds properly.

I have also ported some modifications you suggested from gettext-tools to gettext-runtime (like drop trying to drop CFLAGS=-D_FORTIFY_SOURCE=0 for darwin). hydra will let us know if this is a mistake. If necessary it will be easily to reintroduce.

@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch 2 times, most recently from f64049c to ddea13a Feb 6, 2020
@lsix lsix requested a review from jtojnar Feb 6, 2020
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch from ddea13a to 9af153e Feb 6, 2020
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch from 9af153e to 56b2ad7 Feb 7, 2020
@lsix lsix requested a review from jtojnar Feb 7, 2020
});
] ++ lib.optional stdenv.isDarwin [
./gettext.git-ec0e6b307456ceab352669ae6bccca9702108753.patch
];

outputs = [ "out" "man" "doc" "info" ];

This comment has been minimized.

@jtojnar

jtojnar Feb 7, 2020
Contributor

Could you post tree of each package's output?

Maybe also lib

This comment has been minimized.

@lsix

lsix Feb 7, 2020
Author Member

$ nix-build -A gettext-runtime.out -A gettext-runtime.man -A gettext-runtime.doc -A gettext-runtime.info|xargs tree
/nix/store/akia4d9zd8lmh0dfqzh81zccmrr115y8-gettext-runtime-0.20.1
├── bin
│   ├── envsubst
│   ├── gettext
│   ├── gettext.sh
│   └── ngettext
├── include
│   └── autosprintf.h
├── lib
│   ├── libasprintf.la
│   ├── libasprintf.so -> libasprintf.so.0.0.0
│   ├── libasprintf.so.0 -> libasprintf.so.0.0.0
│   └── libasprintf.so.0.0.0
├── nix-support
│   └── setup-hook
└── share
    ├── gettext
    │   └── ABOUT-NLS
    └── locale
        ├── ast
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── be
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── bg
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ca
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── cs
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── da
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── de
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── el
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── en@boldquot
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── en@quot
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── eo
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── es
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── et
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── fi
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── fr
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ga
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── gl
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── hr
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── hu
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── id
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── it
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ja
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ko
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── nb
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── nl
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── nn
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── pl
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── pt
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── pt_BR
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ro
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── ru
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── sk
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── sl
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── sr
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── sv
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── tr
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── uk
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── vi
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── zh_CN
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        ├── zh_HK
        │   └── LC_MESSAGES
        │       └── gettext-runtime.mo
        └── zh_TW
            └── LC_MESSAGES
                └── gettext-runtime.mo
/nix/store/115fa7117bv4w2sd9ysbwmrj9k9w5z3c-gettext-runtime-0.20.1-man
└── share
    └── man
        ├── man1
        │   ├── envsubst.1.gz
        │   ├── gettext.1.gz
        │   └── ngettext.1.gz
        └── man3
            ├── bindtextdomain.3.gz
            ├── bind_textdomain_codeset.3.gz
            ├── dcgettext.3.gz
            ├── dcngettext.3.gz
            ├── dgettext.3.gz
            ├── dngettext.3.gz
            ├── gettext.3.gz
            ├── ngettext.3.gz
            └── textdomain.3.gz
/nix/store/b3hyj7c3bpk33rbglb451x6kspsbdwfp-gettext-runtime-0.20.1-doc
└── share
    └── doc
        └── gettext-runtime
            ├── autosprintf_all.html
            ├── bindtextdomain.3.html
            ├── bind_textdomain_codeset.3.html
            ├── csharpdoc
            │   ├── begin.html
            │   ├── GNU_Gettext_GettextResourceManager.html
            │   ├── GNU_Gettext_GettextResourceSet.html
            │   ├── GNU_Gettext.html
            │   ├── index.html
            │   └── namespaces.html
            ├── envsubst.1.html
            ├── gettext.1.html
            ├── gettext.3.html
            ├── javadoc2
            │   ├── allclasses-frame.html
            │   ├── deprecated-list.html
            │   ├── gnu
            │   │   └── gettext
            │   │       ├── GettextResource.html
            │   │       ├── package-frame.html
            │   │       ├── package-summary.html
            │   │       └── package-tree.html
            │   ├── help-doc.html
            │   ├── index-all.html
            │   ├── index.html
            │   ├── overview-tree.html
            │   ├── package-list
            │   ├── packages.html
            │   ├── serialized-form.html
            │   └── stylesheet.css
            ├── ngettext.1.html
            ├── ngettext.3.html
            └── textdomain.3.html
/nix/store/4d8pjvwshd41s96kjngmd89j7scpnpyx-gettext-runtime-0.20.1-info
└── share
    └── info
        └── autosprintf.info

102 directories, 94 files
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Feb 7, 2020

For the time being, gettext is turned into an alias to
gettext-runtime to ease transition.

Looking at the tree, gettext should probably be alias of gettext-tools since things depending on it usually just need msgfmt.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Feb 7, 2020

The setup hook should probably be split as well. GETTEXTDATADIRS is used by xgettext and msgfmt IIRC, while the LDFLAGS stuff sounds like it would be runtime thing? IDK.

@lsix
Copy link
Member Author

@lsix lsix commented Feb 7, 2020

Yes, some packages build do require (explicitly) msgfmt.

Do we have a simple way to have a “old style” gettext dependency translate into gettext-runtime as a buildInput and gettext-tools as a nativeBuildInput ?

I am not sure of a nice way to approach that 🤔 suggestions ?

@lsix lsix changed the title gettext: refactor and split in multiple packages WIP: gettext: refactor and split in multiple packages Feb 7, 2020
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Feb 7, 2020

Everything build using meson at least. And instltool should propagate msgfmt. I would be surprised if anything used the runtime.

@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch 2 times, most recently from 45bcd7f to a3233b2 Feb 10, 2020
@ofborg ofborg bot requested a review from 7c6f434c Feb 10, 2020
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch 2 times, most recently from 04e0ae9 to f764d7a Feb 10, 2020
The <package>gettext</package> has been split into 3 components:
<package>gettext-runtime</package>, <package>gettext-tools</package>
and <package>libtextstyle</package>. As a convenience, <package>gettext</package>
now points to <package>gettext-runtime</package>.

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Suggested change
now points to <package>gettext-runtime</package>.
now points to <package>gettext</package>.
@@ -18,7 +18,7 @@ stdenv.mkDerivation rec {
patches = [
(substituteAll {
src = ./fix-test-paths.patch;
inherit coreutils gettext glibcLocales;
inherit coreutils gettext-tools glibcLocales;

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Suggested change
inherit coreutils gettext-tools glibcLocales;
inherit coreutils glibcLocales;
gettext = gettext-tools;

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Bash does not support dashes in variable names.

@@ -35,14 +35,14 @@ index 0a0a28f1..16fd51fe 100755
EOF
mkdir -p ${DIR}/files/de/share/de/LC_MESSAGES
-msgfmt --output-file ${DIR}/files/de/share/de/LC_MESSAGES/helloworld.mo de.po
+@gettext@/bin/msgfmt --output-file ${DIR}/files/de/share/de/LC_MESSAGES/helloworld.mo de.po
+@gettext-tools@/bin/msgfmt --output-file ${DIR}/files/de/share/de/LC_MESSAGES/helloworld.mo de.po

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Suggested change
+@gettext-tools@/bin/msgfmt --output-file ${DIR}/files/de/share/de/LC_MESSAGES/helloworld.mo de.po
+@gettext@/bin/msgfmt --output-file ${DIR}/files/de/share/de/LC_MESSAGES/helloworld.mo de.po
cat > fr.po <<EOF
msgid "Hello world"
msgstr "Bonjour le monde"
EOF
mkdir -p ${DIR}/files/fr/share/fr/LC_MESSAGES
-msgfmt --output-file ${DIR}/files/fr/share/fr/LC_MESSAGES/helloworld.mo fr.po
+@gettext@/bin/msgfmt --output-file ${DIR}/files/fr/share/fr/LC_MESSAGES/helloworld.mo fr.po
+@gettext-tools@/bin/msgfmt --output-file ${DIR}/files/fr/share/fr/LC_MESSAGES/helloworld.mo fr.po

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Suggested change
+@gettext-tools@/bin/msgfmt --output-file ${DIR}/files/fr/share/fr/LC_MESSAGES/helloworld.mo fr.po
+@gettext@/bin/msgfmt --output-file ${DIR}/files/fr/share/fr/LC_MESSAGES/helloworld.mo fr.po
@@ -105,10 +105,10 @@ stdenv.mkDerivation rec {
]);

nativeBuildInputs = [
meson ninja pkgconfig perl python3 gettext gtk-doc docbook_xsl docbook_xml_dtd_45
meson ninja pkgconfig perl python3 gettext-tools gtk-doc docbook_xsl docbook_xml_dtd_45

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

Could you make all the alias fixes into a separate commit so it is easier to review?

libtextstyle = callPackage ../development/libraries/libtextstyle { };

# Ease transition between one-package gettext to split packages (tools - runtime)
gettext = gettext-runtime;

This comment has been minimized.

@jtojnar

jtojnar Feb 10, 2020
Contributor

This is the most common use case.

Suggested change
gettext = gettext-runtime;
gettext = gettext-tools;
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch from f764d7a to 24e7a13 Feb 10, 2020
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Feb 10, 2020

I would actually suggest going with gettext = gettext-tools; and letting Hydra tell us what is broken. And then based on that add the runtime where needed. And finally resolve the alias everywhere.

@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch from 24e7a13 to 429f08e Feb 10, 2020
Following upstream recommendations, gettext is split into 3 distinct
packages:
- gettext-runtime
- gettext-tools
- libtextstyle

See https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=PACKAGING;h=f4bae4561ec70a9de2532502b5c2358bb976036c;hb=55b8fe9a2f12b22eb3ee6baf973e03e60b3a9915

Given that for each sub-package we use `sourceRoot`, the patches do not
apply directly when fetch from upstream git and have to be editted to
strip the top level dir.

For the time being, `gettext` is turned into an alias to
`gettext-runtime` to ease transition.
@lsix lsix force-pushed the lsix:refactor-gettext-0.20.1 branch from 429f08e to 539417b Feb 10, 2020
@lsix
Copy link
Member Author

@lsix lsix commented Feb 13, 2020

@jtojnar I am a but busy at the moment, I’ll come back to this MR probably next week and try to make progress on it.

Thanks for your feedbacks, they are really valuable!

@stale
Copy link

@stale stale bot commented Aug 11, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale label Aug 11, 2020
@Infinisil
Copy link
Member

@Infinisil Infinisil commented Aug 24, 2020

Still interested in this @lsix and @jtojnar?

@stale stale bot removed the 2.status: stale label Aug 24, 2020
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Aug 24, 2020

Yes, we definitely still want this but I do not think I will be able to finish this for 20.09.

@lsix
Copy link
Member Author

@lsix lsix commented Aug 24, 2020

Yes, we still want this, but I have not had much time nor build resources to dedicate to it 😭

@FRidh FRidh modified the milestones: 20.09, 21.03 Aug 24, 2020
@ryantm ryantm marked this pull request as draft Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.