Skip to content

various: fix build with GCC 14#357774

Merged
emilazy merged 7 commits intoNixOS:stagingfrom
FliegendeWurst:shapely-gcc14
Nov 25, 2024
Merged

various: fix build with GCC 14#357774
emilazy merged 7 commits intoNixOS:stagingfrom
FliegendeWurst:shapely-gcc14

Conversation

@FliegendeWurst
Copy link
Member

@FliegendeWurst FliegendeWurst commented Nov 21, 2024

Regression from #356812

Fixes #357975
Fixes #358143

Things done

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Nov 21, 2024
@FliegendeWurst FliegendeWurst changed the title python312Packages.shapely: fix build with GCC 14 various: fix build with GCC 14 Nov 21, 2024
@FliegendeWurst FliegendeWurst force-pushed the shapely-gcc14 branch 2 times, most recently from 6cfe17f to 235fcd1 Compare November 21, 2024 13:05
@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

Can you please check any of these packages can be updated to a new upstream release, or else patched with an upstream change or patch from another GCC 14 distro (e.g. Arch), before we resort to turning off the warnings?

@FliegendeWurst
Copy link
Member Author

FliegendeWurst commented Nov 21, 2024

I will push as soon as it finishes building.

@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

Thank you!

  • linuxPackages.systemtap: 5.0a -> 5.2; libsystemtap: 5.1 -> 5.2 #357981 is currently building the tests on the community builder for SystemTap.

  • Can we bump libmysqlclient? It’s very early in the release cycle.

  • The cdrtools author passed away; I believe https://codeberg.org/schilytools/schilytools is a maintained fork. However it has a somewhat controversial and iffy licensing situation; IIRC we have a bunch of stuff depending on it that probably ought not to. @fabianhjr do I recall correctly that you had a PR to remove at least one use of it?

  • cdrkit (a fork of cdrtools before its licence changed) is dead. A lot of distributions package it, however; perhaps Debian or Fedora or Gentoo might have a patch?

@FliegendeWurst
Copy link
Member Author

Regarding libmysqlclient: @globin, is there a newer version available?

@FliegendeWurst
Copy link
Member Author

Debian has a patch for cdrkit: https://sources.debian.org/patches/cdrkit/9:1.1.11-3.5/fix-implicit-function-declaration.patch/
I'll include it.

@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

Yay. I’d recommend getting it from their Salsa GitLab which IIRC has stabler URLs. Or maybe we should just fetch the whole thing from Salsa, since Debian were basically always the upstream anyway and the actual tarball is gone.

Then we could apply the entire patchset; see e.g. #354257.

@fabianhjr
Copy link
Member

fabianhjr commented Nov 21, 2024

  • The cdrtools author passed away; I believe https://codeberg.org/schilytools/schilytools is a maintained fork. However it has a somewhat controversial and iffy licensing situation; IIRC we have a bunch of stuff depending on it that probably ought not to. @fabianhjr do I recall correctly that you had a PR to remove at least one use of it?

If I remember correctly I changed rhythmbox from brasero to brasero-unwrapped. (Which doesn't depend on cdrtools but brasero wraped does)

EDIT: #345381

@Aleksanaa Aleksanaa changed the title various: fix build with GCC 14 treewide: fix build with GCC 14 Nov 22, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Nov 22, 2024
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

LGTM except for one file and the DBD thing which I’d prefer to omit for now in hopes of finding a better solution.

@FliegendeWurst
Copy link
Member Author

DBD thing which I’d prefer to omit for now in hopes of finding a better solution.

Unfortunately that perl module is used by mariadb-server -> akonadi -> default Plasma setup. So I don't see a much better solution.

@emilazy
Copy link
Member

emilazy commented Nov 23, 2024

Unfortunately that perl module is used by mariadb-server -> akonadi -> default Plasma setup. So I don't see a much better solution.

The rest of the changes seem good, but I would prefer to see if we can solve the libmysqlclient issue before resigning ourselves to simply pinning an old broken version of a Perl package forever. We have over a week and probably multiple until staging-next, there’s no rush.

@emilazy
Copy link
Member

emilazy commented Nov 24, 2024

@Aleksanaa BTW, I believe that “various:” was more accurate than “treewide:” in this case – there’s no way this PR is going to fix all GCC 14 regressions across the tree (and we unnecessarily conflate changes that touch multiple packages with actual treewides, which has lead to miscommunications in the past).

@Aleksanaa Aleksanaa changed the title treewide: fix build with GCC 14 various: fix build with GCC 14 Nov 24, 2024
Comment on lines +7204 to +7216
nativeBuildInputs = [
pkgs.mysql80 # for mysql_config
];
buildInputs = [
DevelChecklib
TestDeep
TestDistManifest
TestPod
pkgs.libmysqlconnectorcpp
pkgs.libxcrypt
pkgs.openssl
pkgs.zstd
];
Copy link
Member

Choose a reason for hiding this comment

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

So, I forget the details of the MySQL/MariaDB split, but: previously we were using libmysqlclient, which is actually mariadb-connector-c. Now we’re pulling in an entire build of actual MySQL and libmysqlconnectorcpp which also seems to be a MySQL thing. I’m not saying this is necessarily a problem but I want to make sure we want to include MySQL in the default desktop closure, and indeed the closure of mariadb-server itself?

There’s also DBD::MariaDB – I don’t know if it’s compatible with mytop or not, but maybe we should be using that instead? cc @Conni2461 @dasJ

Copy link
Contributor

@paparodeo paparodeo Nov 24, 2024

Choose a reason for hiding this comment

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

I think it is needed for mytop.sh and https://github.com/MariaDB/server/blob/eff9c198e32a828f610b93fad3a0f0eb63b3ded2/scripts/mytop.sh#L250-L255 seems to show that it is fine with MariaDB connector

Copy link
Member Author

Choose a reason for hiding this comment

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

I can switch it over, but not really test it.

Copy link
Member

Choose a reason for hiding this comment

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

Switching it over seems like the best idea. I think that we don’t want the MySQL server in the MariaDB server’s build closure, and probably don’t want MySQL stuff in the desktop closure at all :)

Of course I’m assuming that DBD::MariaDB builds…

Copy link
Contributor

Choose a reason for hiding this comment

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

it builds. I switched it over a a few days ago when building but thought that mytop.sh only supported the mysql version so didn't move forward -- looking over it again I misread the mytop.sh code, but like @FliegendeWurst implies, testing that it works is the hard part.

Copy link
Member

Choose a reason for hiding this comment

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

It would be sort of embarrassing if MariaDB were shipping a script that purported to support MariaDB but that actually required MySQL to build, I think. It doesn’t look like the script is doing anything fancy with the connection. I’d be comfortable switching to unbreak it.

Copy link
Contributor

Choose a reason for hiding this comment

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

assuming the DBDMariaDB perl package works, I agree. the diff is pretty straightforward.

diff --git a/pkgs/servers/sql/mariadb/default.nix b/pkgs/servers/sql/mariadb/default.nix
index a178e1fa28e4..921510a07f91 100644
--- a/pkgs/servers/sql/mariadb/default.nix
+++ b/pkgs/servers/sql/mariadb/default.nix
@@ -26,7 +26,7 @@ let
 
     libExt = stdenv.hostPlatform.extensions.sharedLibrary;
 
-    mytopEnv = buildPackages.perl.withPackages (p: with p; [ DBDmysql DBI TermReadKey ]);
+    mytopEnv = buildPackages.perl.withPackages (p: with p; [ DBDMariaDB DBI TermReadKey ]);
 
     common = rec { # attributes common to both builds
       inherit version;

Comment on lines +15 to +18
env.CFLAGS = toString [
"-Wno-error=implicit-int"
"-Wno-error=implicit-function-declaration"
];
Copy link
Member

Choose a reason for hiding this comment

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

Non‐blocking nit: lib.concatStringsSep " " has less surprising behaviour in general (but they are equivalent in this case).

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Looks great now, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants