Skip to content

squeak: fix build with LLVM 16 #268227

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.

Sign up for GitHub

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

Merged
Artturin merged 1 commit into from
Nov 19, 2023
Merged

squeak: fix build with LLVM 16 #268227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 42 additions & 9 deletions pkgs/development/compilers/squeak/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,40 @@
{ lib, stdenv, fetchFromGitHub, fetchurl, fetchzip
, autoconf, automake, autoreconfHook, clang, dos2unix, file, perl
{ lib
, stdenv
, fetchFromGitHub
, fetchurl
, fetchzip
, autoconf
, automake
, autoreconfHook
, dos2unix
, file
, perl
, pkg-config
, alsa-lib, coreutils, freetype, glib, glibc, gnugrep, libGL, libpulseaudio
, libtool, libuuid, openssl, pango, xorg
, squeakImageHash ? null, squeakSourcesHash ? null, squeakSourcesVersion ? null
, squeakVersion ? null, squeakVmCommitHash ? null, squeakVmCommitHashHash ? null
, alsa-lib
, coreutils
, freetype
, glib
, glibc
, gnugrep
, libGL
, libpulseaudio
, libtool
, libuuid
, openssl
, pango
, xorg
, squeakImageHash ? null
, squeakSourcesHash ? null
, squeakSourcesVersion ? null
, squeakVersion ? null
, squeakVmCommitHash ? null
, squeakVmCommitHashHash ? null
, squeakVmVersion ? null
} @ args:

let
inherit (builtins) elemAt;
inherit (builtins) elemAt toString;

nullableOr = o: default: if o == null then default else o;

bits = stdenv.hostPlatform.parsed.cpu.bits;
Expand Down Expand Up @@ -75,7 +100,6 @@ in stdenv.mkDerivation {
autoconf
automake
autoreconfHook
clang
dos2unix
file
perl
Expand Down Expand Up @@ -140,7 +164,16 @@ in stdenv.mkDerivation {
# Workaround build failure on -fno-common toolchains:
# ld: vm/vm.a(cogit.o):spur64src/vm/cogitX64SysV.c:2552: multiple definition of
# `traceStores'; vm/vm.a(gcc3x-cointerp.o):spur64src/vm/cogit.h:140: first defined here
env.NIX_CFLAGS_COMPILE = "-fcommon";
env.NIX_CFLAGS_COMPILE = toString (
[ "-fcommon" ]
++ (lib.optionals stdenv.cc.isClang [
# LLVM 16 turned these into errors (rightly, perhaps.)
# Allow this package to continue to build despite this change.
"-Wno-error=int-conversion"
"-Wno-error=implicit-function-declaration"
"-Wno-error=incompatible-function-pointer-types"
])
);
Copy link

@ghost ghost Nov 18, 2023

Choose a reason for hiding this comment

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

I would prefer this to be a list at NIX_CFLAGS_COMPILE rather than a string at env.NIX_CFLAGS_COMPILE, unless we have a policy of using env.«…» that I don't know about.

Copy link
Contributor Author

@philiptaron philiptaron Nov 18, 2023

Choose a reason for hiding this comment

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

env.NIX_CFLAGS_COMPILE is definitely the majority. I also prefer NIX_CFLAGS_COMPILE, and I'll change it to that form as it works just fine.

$ rg env.NIX_CFLAGS_COMPILE | wc -l
816

$ rg NIX_CFLAGS_COMPILE | rg -v env | wc -l
236

Copy link
Member

@wegank wegank Nov 18, 2023

Choose a reason for hiding this comment

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

No, please don't. Moving environmental variables to env is a step towards __structuredAttrs support, see #263773.

Copy link
Member

@wegank wegank Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

> $ rg NIX_CFLAGS_COMPILE | rg -v env | wc -l
> 236

We've just done #217206 this year, it's unbelievable there are still that many instances of NIX_CFLAGS_COMPILE. Does the command count patterns like the following?

  env = {
    NIX_CFLAGS_COMPILE = "...";
  };

Copy link
Contributor Author

@philiptaron philiptaron Nov 18, 2023

Choose a reason for hiding this comment

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

It would count those, yes. There's likely quite a few false positives.

Copy link

@ghost ghost Nov 19, 2023

Choose a reason for hiding this comment

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

Believe it because it's an undocumented practice - https://nixos.org/manual/nixpkgs/unstable/.


preAutoreconf = ''
pushd ./platforms/unix/config > /dev/null
Expand Down
4 changes: 3 additions & 1 deletion pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17287,7 +17287,9 @@ with pkgs;
inherit (darwin.apple_sdk.frameworks) SystemConfiguration CoreFoundation Security;
};

squeak = callPackage ../development/compilers/squeak { };
squeak = callPackage ../development/compilers/squeak {
stdenv = clangStdenv;
Copy link

@ghost ghost Nov 18, 2023

Choose a reason for hiding this comment

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

Does this need to be clangStdenv other than for testing?

Copy link
Contributor Author

@philiptaron philiptaron Nov 18, 2023

Choose a reason for hiding this comment

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

When omitted, nix-build -A squeak errors as follows:

Configuring Squeak 5.0-202003021730 (5.3-19459-64bit) for x86_64-pc-linux-gnu

checking whether make sets $(MAKE)... yes
checking for gcc... clang
checking whether the C compiler works... no
configure: error: in `/build/source/build.linux64x64/squeak.cog.spur/build':
configure: error: C compiler cannot create executables
See `config.log' for more details

I don't know why, in the default stdenv, it detects clang, as it's no longer passed into the environment. If I pass in gccStdenv, clang is still detected.

So, yes, I think this does have to be clangStdenv.

};

squirrel-sql = callPackage ../development/tools/database/squirrel-sql {
drivers = [ jtds_jdbc mssql_jdbc mysql_jdbc postgresql_jdbc ];
Expand Down