mlterm: unbreak #370949
mlterm: unbreak #370949
Conversation
gcc14 would throw:
```
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../uim -I../replace -I../uim -g -O2 -pedantic -pipe -Wall -Wchar-subscripts -Wmissing-declarations -Wredundant-decls -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wsign-compare -Wno-long-long -Wno-overlength-strings -DNDEBUG -c bsd-snprintf.c -fPIC -DPIC -o .libs/bsd-snprintf.o
bsd-snprintf.c: In function 'dopr':
bsd-snprintf.c:104:38: error: assignment to expression with array type
104 | # define VA_COPY(dest, src) (dest) = (src)
| ^
bsd-snprintf.c:186:9: note: in expansion of macro 'VA_COPY'
186 | VA_COPY(args, args_in);
| ^~~~~~~
```
and I don't have the motivation to debug autohell crap.
Most of the time is wasted in autohell scripts anyays and it doesn't work: the build fails to relink some objects. Explicitly disable parallel building.
It fails to compile in a way that is probably only comprehensible to exactly one person on earth which is upstream.
github-actions
bot
added
10.rebuild-darwin: 0
| @@ -1,6 +1,7 @@ | |||
| { | |||
| args@{ | |||
There was a problem hiding this comment.
You couldn't refer to the args' stdenv otherwise.
| @@ -1,5 +1,6 @@ | |||
| { | |||
| args@{ | |||
There was a problem hiding this comment.
I don't like this pattern TBH, I prefer using stdenv'...
There was a problem hiding this comment.
I don't like it much but this is preferable better to the diff noise caused by replacing all references to stdenv with stdenv' IMHO.
There was a problem hiding this comment.
I don't like it much but this is preferable better to the diff noise caused by replacing all references to
stdenvwithstdenv'IMHO.
Isn't it just 1 line? It seems like it should either be this line or the mkDerivation line.
There was a problem hiding this comment.
stdenv is usually used in multiple places for stdenv.is* guards etc. I didn't check for this particular package; I just apply this pattern because it works everywhere.
| @@ -53,16 +53,20 @@ assert withNetworking -> curl != null && openssl != null; | |||
| assert withFFI -> libffi != null; | |||
| assert withMisc -> libeb != null; | |||
|
|
|||
| let | |||
| stdenv = if args.stdenv.cc.isGNU then args.gcc13Stdenv else args.stdenv; | |||
There was a problem hiding this comment.
You can apply uim/uim@99fd890 instead.
| @@ -98,6 +99,7 @@ let | |||
| commaSepList = lib.concatStringsSep "," (builtins.attrNames (lib.filterAttrs (n: v: v) attrset)); | |||
| in | |||
| lib.withFeatureAs (commaSepList != "") featureName commaSepList; | |||
| stdenv = if args.stdenv.cc.isGNU then args.gcc13Stdenv else args.stdenv; | |||
There was a problem hiding this comment.
It does not look like anything more elaborate than -Wno-error=incompatible-pointer-types would be required to fix compilation, which is far preferable to pinning the old GCC. However, upstream has already fixed these issues in e.g. arakiken/mlterm@3d38b72 and arakiken/mlterm@821a556. So let’s just apply the patches instead.
|
Honestly I'd rather just use keep these on gcc13 until they're updated. That's by far the easier option and have you looked at how huge those mlterm patches are? I'll try to apply them but if it doesn't just immediately work, I'd just rather just not. |
|
Yeah the UIM patch doesn't work and the mlterm patches don't apply. I'd rather just not. |
|
Well, the UIM patch is a one‐liner, but I assume you need For |
|
Sorry but I fail to see how doing all of that is better than just simply using gcc13 for a little longer. Feel free to implement that and revert this PR when you've done that. In the mean time, I'd like to have this merged and packages unbroken. Using git HEAD also sounds like an inappropriate solution to me. |
|
I don’t think one patch, an To be honest this attitude makes me want to not bother continuing to spend huge amounts of effort on Nixpkgs compiler upgrades. A Red Hat employee already did the work of writing patches for these issues and upstreaming them en masse for us to benefit from but in Nixpkgs we can’t even do code review to suggest something that takes a handful more lines without getting dismissed? |
|
As mentioned, the patches do not work. I also don't see why this should discourage you from doing compiler upgrades; this is just the fallout of it and that's up to individual package maintainers to handle themselves which, in this case, is me. Unless you have another solution that isn't just a giant PITA, I plan to merge this as is. I'll add FIXMEs though. |
|
I too, as a package maintainer, would prefer to use patches / Git head revisions with which the latest compiler can be used. We could also press upstream to tag a new release so that it'd make even more sense to use these revisions. |
|
Here’s the simplest possible diff that gets things building as before with no patches (though I’d prefer an diff --git a/pkgs/applications/terminal-emulators/mlterm/default.nix b/pkgs/applications/terminal-emulators/mlterm/default.nix
index 95d63d7724..adb4d7bba1 100644
--- a/pkgs/applications/terminal-emulators/mlterm/default.nix
+++ b/pkgs/applications/terminal-emulators/mlterm/default.nix
@@ -1,6 +1,5 @@
-args@{
+{
stdenv,
- gcc13Stdenv,
lib,
fetchFromGitHub,
pkg-config,
@@ -99,7 +98,6 @@
commaSepList = lib.concatStringsSep "," (builtins.attrNames (lib.filterAttrs (n: v: v) attrset));
in
lib.withFeatureAs (commaSepList != "") featureName commaSepList;
- stdenv = if args.stdenv.cc.isGNU then args.gcc13Stdenv else args.stdenv;
in
stdenv.mkDerivation (finalAttrs: {
pname = "mlterm";
@@ -185,9 +183,11 @@
--replace "-m 4755 -o root" " "
'';
- env = lib.optionalAttrs stdenv.cc.isClang {
- NIX_CFLAGS_COMPILE = "-Wno-error=int-conversion -Wno-error=incompatible-function-pointer-types";
- };
+ env.NIX_CFLAGS_COMPILE = lib.concatStringsSep " " [
+ "-Wno-error=int-conversion"
+ "-Wno-error=incompatible-pointer-types"
+ "-Wno-error=implicit-function-declaration"
+ ];
configureFlags =
[
diff --git a/pkgs/tools/inputmethods/uim/default.nix b/pkgs/tools/inputmethods/uim/default.nix
index 94fac0cb44..317131b052 100644
--- a/pkgs/tools/inputmethods/uim/default.nix
+++ b/pkgs/tools/inputmethods/uim/default.nix
@@ -1,7 +1,6 @@
-args@{
+{
lib,
stdenv,
- gcc13Stdenv,
fetchFromGitHub,
shared-mime-info,
autoconf,
@@ -53,10 +52,6 @@
assert withFFI -> libffi != null;
assert withMisc -> libeb != null;
-let
- stdenv = if args.stdenv.cc.isGNU then args.gcc13Stdenv else args.stdenv;
-in
-
stdenv.mkDerivation rec {
version = "1.8.9";
pname = "uim";
@@ -172,6 +167,8 @@
dontUseCmakeConfigure = true;
+ env.NIX_CFLAGS_COMPILE = "-Wno-error=implicit-function-declaration";
+
meta = with lib; {
homepage = src.meta.homepage;
description = "Multilingual input method framework";Here’s what’s required to get UIM up to date with the GCC 14 diff --git a/pkgs/tools/inputmethods/uim/data-hook.patch b/pkgs/tools/inputmethods/uim/data-hook.patch
index be80962527..9ae83b4f29 100644
--- a/pkgs/tools/inputmethods/uim/data-hook.patch
+++ b/pkgs/tools/inputmethods/uim/data-hook.patch
@@ -1,38 +1,40 @@
---- a/gtk2/immodule/Makefile.in 2015-11-24 16:21:08.967087208 +0900
-+++ b/gtk2/immodule/Makefile.in 2015-11-24 16:22:53.316095150 +0900
-@@ -928,7 +928,6 @@
-
- install-data-am: install-moduleLTLIBRARIES
- @$(NORMAL_INSTALL)
-- $(MAKE) $(AM_MAKEFLAGS) install-data-hook
- install-dvi: install-dvi-am
-
- install-dvi-am:
-@@ -993,7 +992,7 @@
- distclean-compile distclean-generic distclean-libtool \
- distclean-tags distdir dvi dvi-am html html-am info info-am \
- install install-am install-data install-data-am \
-- install-data-hook install-dvi install-dvi-am install-exec \
-+ install-dvi install-dvi-am install-exec \
- install-exec-am install-html install-html-am install-info \
- install-info-am install-man install-moduleLTLIBRARIES \
- install-pdf install-pdf-am install-ps install-ps-am \
---- a/gtk3/immodule/Makefile.in 2015-11-24 16:21:08.971087209 +0900
-+++ b/gtk3/immodule/Makefile.in 2015-11-24 16:23:28.251097832 +0900
-@@ -896,7 +896,6 @@
-
- install-data-am: install-moduleLTLIBRARIES
- @$(NORMAL_INSTALL)
-- $(MAKE) $(AM_MAKEFLAGS) install-data-hook
- install-dvi: install-dvi-am
-
- install-dvi-am:
-@@ -959,7 +958,7 @@
- cscopelist-am ctags ctags-am distclean distclean-compile \
- distclean-generic distclean-libtool distclean-tags distdir dvi \
- dvi-am html html-am info info-am install install-am \
-- install-data install-data-am install-data-hook install-dvi \
-+ install-data install-data-am install-dvi \
- install-dvi-am install-exec install-exec-am install-html \
- install-html-am install-info install-info-am install-man \
- install-moduleLTLIBRARIES install-pdf install-pdf-am \
+diff --git a/gtk2/immodule/Makefile.am b/gtk2/immodule/Makefile.am
+index d219a5a4b0..331e127122 100644
+--- a/gtk2/immodule/Makefile.am
++++ b/gtk2/immodule/Makefile.am
+@@ -37,6 +37,7 @@
+ GTK_RC_GET_IMMODULE_FILE = $(top_builddir)/gtk2/immodule/gtk-rc-get-immodule-file
+ QUERY_COMMAND = gtk-query-immodules-2.0
+
++if FALSE
+ install-data-hook: gtk-rc-get-immodule-file
+ if test -z $(DESTDIR); then \
+ if test $(libdir) = $(GTK_LIBDIR); then \
+@@ -82,6 +83,7 @@
+ fi \
+ fi \
+ fi
++endif
+ else
+ install-data-hook:
+
+diff --git a/gtk3/immodule/Makefile.am b/gtk3/immodule/Makefile.am
+index de58b88916..d519b03b1d 100644
+--- a/gtk3/immodule/Makefile.am
++++ b/gtk3/immodule/Makefile.am
+@@ -39,6 +39,7 @@
+
+ QUERY_COMMAND = gtk-query-immodules-3.0
+
++if FALSE
+ install-data-hook:
+ if test -z $(DESTDIR); then \
+ if test $(libdir) = $(GTK3_LIBDIR); then \
+@@ -74,6 +75,7 @@
+ fi \
+ fi \
+ fi
++endif
+ else
+ install-data-hook:
+
diff --git a/pkgs/tools/inputmethods/uim/default.nix b/pkgs/tools/inputmethods/uim/default.nix
index 317131b052..e89f059d11 100644
--- a/pkgs/tools/inputmethods/uim/default.nix
+++ b/pkgs/tools/inputmethods/uim/default.nix
@@ -2,6 +2,7 @@
lib,
stdenv,
fetchFromGitHub,
+ fetchpatch,
shared-mime-info,
autoconf,
automake,
@@ -103,7 +104,7 @@
++ lib.optional withFFI libffi
++ lib.optional withMisc libeb;
- prePatch = ''
+ postPatch = ''
patchShebangs *.sh */*.sh */*/*.sh
# configure sigscheme in maintainer mode or else some function tables won't get autogenerated
@@ -118,6 +119,18 @@
'';
patches = [
+ (fetchpatch {
+ name = "configure-c99-1.patch";
+ url = "https://github.com/uim/uim/commit/99fd890fa601b81ff99e5e0f1977fe309f56b90e.patch";
+ hash = "sha256-j87/Kor5mDqULnL7uors6oOAyf0+Zw0/z/TbRWP+5i4=";
+ })
+
+ (fetchpatch {
+ name = "configure-c99-2.patch";
+ url = "https://github.com/uim/uim/commit/378853ab52c6857a2794d63fd1d2deb57afb20c0.patch";
+ hash = "sha256-AKu/CoW+YryJ/GZIAa+yWqSpAxkPbRmxtWyebLvByEQ=";
+ })
+
./data-hook.patch
];
@@ -167,8 +180,6 @@
dontUseCmakeConfigure = true;
- env.NIX_CFLAGS_COMPILE = "-Wno-error=implicit-function-declaration";
-
meta = with lib; {
homepage = src.meta.homepage;
description = "Multilingual input method framework";And finally, thanks to the effort already put in by Debian maintainers, we don’t need to do any manual work to backport the upstream diff --git a/pkgs/applications/terminal-emulators/mlterm/default.nix b/pkgs/applications/terminal-emulators/mlterm/default.nix
index adb4d7bba1..82a40691c7 100644
--- a/pkgs/applications/terminal-emulators/mlterm/default.nix
+++ b/pkgs/applications/terminal-emulators/mlterm/default.nix
@@ -2,6 +2,7 @@
stdenv,
lib,
fetchFromGitHub,
+ fetchpatch,
pkg-config,
autoconf,
makeDesktopItem,
@@ -110,6 +111,25 @@
sha256 = "sha256-gfs5cdwUUwSBWwJJSaxrQGWJvLkI27RMlk5QvDALEDg=";
};
+ patches = [
+ (fetchpatch {
+ name = "mlterm-configure-implicit-function-declaration.patch";
+ url = "https://github.com/arakiken/mlterm/commit/1a9ee97e4574c5892bf12090b812b0538dcdf8f2.patch";
+ hash = "sha256-Kk+x5LAq+beZWE8yj5WfdS82ConLSgxNquzQd5mvOA4=";
+ })
+
+ (fetchpatch {
+ name = "mlterm-wayland-implicit-function-declaration.patch";
+ url = "https://github.com/arakiken/mlterm/commit/20ab931d5055dc5835154a75ca672fade478549f.patch";
+ hash = "sha256-rDmQ0e3dQD7UAGTX4ljOrDqTTddBqvnnRFnqDjRLAss=";
+ })
+
+ (fetchpatch {
+ url = "https://salsa.debian.org/debian/mlterm/-/raw/d9b1555e9220985e0c89a6ff5a0d58f7b18cc123/debian/patches/fix-incompat-pointer-types.patch";
+ hash = "sha256-EcI15FjQfcN8pcE1MqsBfaHQ4j+gyoeesN/WoHb7WnU=";
+ })
+ ];
+
nativeBuildInputs =
[
pkg-config
@@ -183,12 +203,6 @@
--replace "-m 4755 -o root" " "
'';
- env.NIX_CFLAGS_COMPILE = lib.concatStringsSep " " [
- "-Wno-error=int-conversion"
- "-Wno-error=incompatible-pointer-types"
- "-Wno-error=implicit-function-declaration"
- ];
-
configureFlags =
[
(withFeaturesList "type-engines" enableTypeEngines)Anyway. If we just wanted a higher GCC version and for all packages to build with the minimum effort, we could have turned off the new default warnings. We did not, because those warnings point to actual problems in the code and because part of the task of a distribution is to ensure its packages are in a healthy state and move the ecosystem ahead. In fact, we tried to explicitly enable the GCC 14 warnings even on GCC 13 when we were having issues with the GCC 14 upgrade a while ago, to ensure that we could prepare for the final bump. If we pin an old GCC every time we run into basic compilation failures, then either we effectively do not bump the compiler, or it simply defers the problems to a later date (and another person – frequently me). GCC 14 came out eight months ago, and Florian Weimer has been sending fixes for its then‐upcoming changes upstream for years now, as in the case of the UIM patch I linked to from over two years ago. The last UIM release was almost 2½ years ago; the last I shepherded extensive testing and fixes of GCC 14 and LLVM 19 in This is the first PR I’ve reviewed for this cycle that pinned an old compiler without any effort to even try disabling warning flags or applying upstream patches, and the first one where my review suggesting a better approach with pointers was dismissed from the PR with “WONTFIX” right after explicitly offering to put in time to help with any issues. I’m not saying that I expect people to go to a great amount of effort debugging everything themselves and fixing up old patches if a quick check for upstream patches and warning flags to toggle off doesn’t work – I’m happy to share my expertise from handling a lot of these issues and put effort in to quickly fix packages, and I’ll readily agree that sometimes turning off the warning flags and waiting for upstream is the only reasonable fix – but I can only help out if maintainers want to cooperate. I hope you will find one of my diffs acceptable, but more than that I hope you’ll think about how this kind of code review dynamic affects the people who put in the most cumulative effort to working to ensure Nixpkgs keeps up with core toolchain changes. Working on |
|
Thanks for the detailed response @emilazy. I fully support your opinion. A few notes to add: Mlterm hopefully will get a new release soon. I subscribed to this issue: Until then, I created an alternative PR for the fixes requested here at: And there, I used |
| @@ -176,6 +168,8 @@ stdenv.mkDerivation rec { | |||
| export XDG_DATA_DIRS="${shared-mime-info}/share" | |||
| ''; | |||
|
|
|||
| enableParallelBuilding = false; | |||
|
|
|||
| dontUseCmakeConfigure = true; | |||
|
|
|||
| meta = with lib; { | |||
| @@ -176,6 +168,8 @@ stdenv.mkDerivation rec { | |||
| export XDG_DATA_DIRS="${shared-mime-info}/share" | |||
| ''; | |||
|
|
|||
| enableParallelBuilding = false; | |||
|
|
|||
| dontUseCmakeConfigure = true; | |||
|
|
|||
There was a problem hiding this comment.
passthru.updateScript = gitUpdater { rev-prefix = "v"; };
| pname = "uim"; | ||
|
|
||
| src = fetchFromGitHub { | ||
| owner = "uim"; | ||
| repo = "uim"; | ||
| rev = "2c0958c9c505a87e70e344c2192e2e5123c71ea5"; |
There was a problem hiding this comment.
rev -> tag
Is this new ever since #368177 ? How come that PR didn't change uim?
|
|
Aish this was by mistake, intended to merge my PR: #371141 . |
OK. I am working in parallel in a treewide solution for this. I need to setup some CI tho. |
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.