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.

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

Jump to bottom

Make recipes consistent w.r. libintl, iconv, gettext #35450

Merged
merged 2 commits into from May 3, 2023
Merged

Make recipes consistent w.r. libintl, iconv, gettext #35450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc844a2
Rationalize support of libiconv and gettext/libintl vs libc
greenc-FNAL Apr 19, 2023
640593b
Requested changes per @scheibelp review
greenc-FNAL May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions var/spack/repos/builtin/packages/acl/package.py
View file
Open in desktop
Expand Up @@ -26,8 +26,10 @@ class Acl(AutotoolsPackage):
depends_on("attr")
depends_on("gettext")

def setup_build_environment(self, env):
env.append_flags("LDFLAGS", "-lintl")
def flag_handler(self, name, flags):
if name == "ldlibs" and "intl" in self.spec["gettext"].libs.names:
flags.append("-lintl")
return self.build_system_flags(name, flags)

def autoreconf(self, spec, prefix):
bash = which("bash")
Expand Down
3 changes: 2 additions & 1 deletion var/spack/repos/builtin/packages/apex/package.py
View file
Open in desktop
Expand Up @@ -154,7 +154,8 @@ def cmake_args(self):
args.append("-DBFD_ROOT={0}".format(spec["binutils"].prefix))

if "+binutils ^binutils+nls" in spec:
args.append("-DCMAKE_SHARED_LINKER_FLAGS=-lintl")
if "intl" in self.spec["gettext"].libs.names:
args.append("-DCMAKE_SHARED_LINKER_FLAGS=-lintl")

if "+otf2" in spec:
args.append("-DOTF2_ROOT={0}".format(spec["otf2"].prefix))
Expand Down
10 changes: 7 additions & 3 deletions var/spack/repos/builtin/packages/bash/package.py
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
import re

from spack.package import *
from spack.util.environment import is_system_path


class Bash(AutotoolsPackage, GNUMirrorPackage):
Expand Down Expand Up @@ -177,17 +178,20 @@ def determine_version(cls, exe):

def configure_args(self):
spec = self.spec

return [
args = [
# https://github.com/Homebrew/legacy-homebrew/pull/23234
# https://trac.macports.org/ticket/40603
"CFLAGS=-DSSH_SOURCE_BASHRC",
"LIBS=" + spec["ncurses"].libs.link_flags,
"--with-curses",
"--enable-readline",
"--with-installed-readline",
"--with-libiconv-prefix={0}".format(spec["iconv"].prefix),
]
if spec["iconv"].name == "libc":
args.append("--without-libiconv-prefix")
elif not is_system_path(spec["iconv"].prefix):
args.append("--with-libiconv-prefix={0}".format(spec["iconv"].prefix))
return args

def check(self):
make("tests")
Expand Down
10 changes: 5 additions & 5 deletions var/spack/repos/builtin/packages/bcache/package.py
View file
Open in desktop
Expand Up @@ -24,16 +24,16 @@ class Bcache(MakefilePackage):
depends_on("gettext")
depends_on("pkgconfig", type="build")

def setup_build_environment(self, env):
# Add -lintl if provided by gettext, otherwise libintl is provided by the system's glibc:
if "gettext" in self.spec and "intl" in self.spec["gettext"].libs.names:
env.append_flags("LDFLAGS", "-lintl")

patch(
"func_crc64.patch",
sha256="558b35cadab4f410ce8f87f0766424a429ca0611aa2fd247326ad10da115737d",
)

def flag_handler(self, name, flags):
if name == "ldflags" and "intl" in self.spec["gettext"].libs.names:
flags.append("-lintl")
return self.env_flags(name, flags)

def install(self, spec, prefix):
mkdirp(prefix.bin)
install("bcache-register", prefix.bin)
Expand Down
2 changes: 1 addition & 1 deletion var/spack/repos/builtin/packages/bind9/package.py
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@ class Bind9(AutotoolsPackage):
depends_on("libuv", type="link")
depends_on("pkgconfig", type="build")
depends_on("openssl", type="link")
depends_on("libiconv", type="link")
depends_on("iconv", type="link")

def configure_args(self):
args = [
Expand Down
15 changes: 9 additions & 6 deletions var/spack/repos/builtin/packages/binutils/package.py
View file
Open in desktop
Expand Up @@ -269,9 +269,12 @@ def install_headers(self):
# also grab the headers from the bfd directory
install(join_path(self.build_directory, "bfd", "*.h"), extradir)

def setup_build_environment(self, env):
if self.spec.satisfies("%cce"):
env.append_flags("LDFLAGS", "-Wl,-z,muldefs")

if "+nls" in self.spec:
env.append_flags("LDFLAGS", "-lintl")
def flag_handler(self, name, flags):
spec = self.spec
if name == "ldflags":
if spec.satisfies("%cce"):
flags.append("-Wl,-z,muldefs")
elif name == "ldlibs":
if "+nls" in self.spec and "intl" in self.spec["gettext"].libs.names:
flags.append("-lintl")
return self.build_system_flags(name, flags)
scheibelp marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 19 additions & 3 deletions var/spack/repos/builtin/packages/elfutils/package.py
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@
import os.path

from spack.package import *
from spack.util.environment import is_system_path


class Elfutils(AutotoolsPackage, SourcewarePackage):
Expand Down Expand Up @@ -99,7 +100,12 @@ def patch(self):
files = glob.glob(os.path.join("*", "Makefile.in"))
filter_file("-Werror", "", *files)

flag_handler = AutotoolsPackage.build_system_flags
def flag_handler(self, name, flags):
if name == "ldlibs":
spec = self.spec
if "+nls" in spec and "intl" in spec["gettext"].libs.names:
flags.append("-lintl")
return self.inject_flags(name, flags)
scheibelp marked this conversation as resolved.
Show resolved Hide resolved

def configure_args(self):
spec = self.spec
Expand All @@ -120,9 +126,19 @@ def configure_args(self):
# zlib is required
args.append("--with-zlib=%s" % spec["zlib"].prefix)

if spec.satisfies("@0.183:"):
if spec["iconv"].name == "libc":
args.append("--without-libiconv-prefix")
elif not is_system_path(spec["iconv"].prefix):
args.append("--with-libiconv-prefix=" + format(spec["iconv"].prefix))

if "+nls" in spec:
# configure doesn't use LIBS correctly
args.append("LDFLAGS=-Wl,--no-as-needed -L%s -lintl" % spec["gettext"].prefix.lib)
# Prior to 0.183, only msgfmt is used from gettext.
if spec.satisfies("@0.183:"):
if "intl" not in spec["gettext"].libs.names:
args.append("--without-libintl-prefix")
elif not is_system_path(spec["gettext"].prefix):
args.append("--with-libintl-prefix=" + spec["gettext"].prefix)
else:
args.append("--disable-nls")

Expand Down
12 changes: 9 additions & 3 deletions var/spack/repos/builtin/packages/extrae/package.py
View file
Open in desktop
Expand Up @@ -127,14 +127,20 @@ def configure_args(self):
make.add_default_arg("CXXFLAGS=%s" % self.compiler.cxx11_flag)
args.append("CXXFLAGS=%s" % self.compiler.cxx11_flag)

return args

def flag_handler(self, name, flags):
# This was added due to:
# - configure failure
# https://www.gnu.org/software/gettext/FAQ.html#integrating_undefined
# - linking error
# https://github.com/bsc-performance-tools/extrae/issues/57
args.append("LDFLAGS=-lintl -pthread")
scheibelp marked this conversation as resolved.
Show resolved Hide resolved

return args
if name == "ldlibs":
if "intl" in self.spec["gettext"].libs.names:
flags.append("-lintl")
elif name == "ldflags":
flags.append("-pthread")
return self.build_system_flags(name, flags)
Comment on lines +138 to +143
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant to https://github.com/spack/spack/pull/35450/files#r1164714416


def install(self, spec, prefix):
with working_dir(self.build_directory):
Expand Down
2 changes: 1 addition & 1 deletion var/spack/repos/builtin/packages/ffmpeg/package.py
View file
Open in desktop
Expand Up @@ -75,7 +75,7 @@ class Ffmpeg(AutotoolsPackage):
variant("libx264", default=False, description="H.264 encoding")

depends_on("alsa-lib", when="platform=linux")
depends_on("libiconv")
depends_on("iconv")
depends_on("yasm@1.2.0:")
depends_on("zlib")

Expand Down
4 changes: 2 additions & 2 deletions var/spack/repos/builtin/packages/fsl/package.py
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ class Fsl(Package, CudaPackage):
depends_on("expat")
depends_on("libx11")
depends_on("glu")
depends_on("libiconv")
depends_on("iconv")
depends_on("openblas", when="@6:")
depends_on("vtk@:8")

Expand All @@ -40,7 +40,7 @@ class Fsl(Package, CudaPackage):

patch("build_log.patch")
patch("eddy_Makefile.patch", when="@6.0.4")
patch("iconv.patch")
patch("iconv.patch", when="^libiconv")
patch("fslpython_install_v5.patch", when="@:5")
patch("fslpython_install_v604.patch", when="@6.0.4")
patch("fslpython_install_v605.patch", when="@6.0.5")
Expand Down
8 changes: 6 additions & 2 deletions var/spack/repos/builtin/packages/gdal/package.py
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@
from spack.build_systems.autotools import AutotoolsBuilder
from spack.build_systems.cmake import CMakeBuilder
from spack.package import *
from spack.util.environment import filter_system_paths
from spack.util.environment import filter_system_paths, is_system_path


class Gdal(CMakePackage, AutotoolsPackage, PythonExtension):
Expand Down Expand Up @@ -626,7 +626,6 @@ def configure_args(self):
self.with_or_without("hdf4", package="hdf"),
self.with_or_without("hdf5", package="hdf5"),
self.with_or_without("hdfs", package="hadoop"),
self.with_or_without("libiconv-prefix", variant="iconv", package="iconv"),
Copy link
Member

Choose a reason for hiding this comment

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

What's the build error you see without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

If iconv is provided by libc and is an external, then this adds /usr as a high-priority prefix.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like it isn't a bug with libc but a bug with external packages. Shouldn't you instead check where iconv is installed and filter out system paths? spack.util.environment has is_system_path and filter_system_paths functions to handle this kind of stuff.

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 need to double-check, but I'm moderately sure that using --libiconv-prefix tries to add -liconv to the library list which will fail if we're getting it from libc. The fouling of the library search path is secondary to that.

With reference to, "Shouldn't you instead check where iconv is installed and filter out system paths?" however: if "you" is, the package maintainer, the unfortunate answer to that is, "no, because nobody else does." As a release manager who configures a certain set of externals all the time, I'm forever running into packages where some aspect of the recipe: library search paths, PATH, or other environment settings gets tripped up by using externals. It's left to individual Spack users like me to trip over the problem, fix it in every package that has the problem and submit the PR. If there were a way to have the CI test recipe builds with dependencies configured as externals in addition to having them installed in the same Spack setup, that might help. Until then however, the burden falls on the first unfortunate soul to want to build against that dependency as an external and gets to watch everything explode.

Copy link
Member

Choose a reason for hiding this comment

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

By "you" I mean the person writing or maintaining the package.py build recipes. From your initial reply, it sounded like instead of checking whether iconv is libc, we should instead check to see if iconv is installed in a system directory where many other things are installed.

We automatically filter system deps most of the time (anything automatically added to the compiler wrappers, for example). It's hard to automatically filter them the rest of the time though. The best we can do automatically would be to ensure that all link/include flags are sorted with system locations last. Can't remember if @scheibelp or @becker33 or @haampie ever managed to get that working, or who was working on that. I agree that it's annoying to have to do this on a package-by-package basis.

Copy link
Member Author

Choose a reason for hiding this comment

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

By "you" I mean the person writing or maintaining the package.py build recipes. From your initial reply, it sounded like instead of checking whether iconv is libc, we should instead check to see if iconv is installed in a system directory where many other things are installed.

No, sorry.

We automatically filter system deps most of the time (anything automatically added to the compiler wrappers, for example). It's hard to automatically filter them the rest of the time though. The best we can do automatically would be to ensure that all link/include flags are sorted with system locations last. Can't remember if @scheibelp or @becker33 or @haampie ever managed to get that working, or who was working on that. I agree that it's annoying to have to do this on a package-by-package basis.

We already have environment utilities provided by me to do this:

spack/lib/spack/spack/util/environment.py

Lines 76 to 80 in 9b3c4e0

def deprioritize_system_paths(paths):
"""Put system paths at the end of paths, otherwise preserving order."""
filtered_paths = filter_system_paths(paths)
fp = set(filtered_paths)
return filtered_paths + [p for p in paths if p not in fp]
and

spack/lib/spack/spack/util/environment.py

Lines 330 to 338 in 9b3c4e0

class DeprioritizeSystemPaths(NameModifier):
def execute(self, env):
tty.debug("DeprioritizeSystemPaths: {0}".format(self.name), level=3)
environment_value = env.get(self.name, "")
directories = environment_value.split(self.separator) if environment_value else []
directories = deprioritize_system_paths(
[path_to_os_path(os.path.normpath(x)).pop() for x in directories]
)
env[self.name] = self.separator.join(directories)
but that has to be used on a package-by-package basis. I'm not sure how easy that would be to integrate into the wrapper's operations.

Copy link
Member

@haampie haampie Feb 20, 2023 •
edited

Choose a reason for hiding this comment

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

It's already used by default.

Copy link
Member

Choose a reason for hiding this comment

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

@greenc-FNAL I'll note that our compiler wrappers reorder -L etc. so even if the configure script introduces a mixed order of system/custom paths, our wrapper should disentangle that. It would be useful to see an example failed installation and specifically the corresponding modified compiler invocations you can see in spack-cc..out if you run the install with -d like spack -d install ....

self.with_or_without("idb", package="idb"),
self.with_or_without("ingres", package="ingres"),
self.with_or_without("jasper", package="jasper"),
Expand Down Expand Up @@ -680,6 +679,11 @@ def configure_args(self):
self.with_or_without("perl"),
self.with_or_without("php"),
]
if "+iconv" in self.spec:
if self.spec["iconv"].name == "libc":
args.append("--without-libiconv-prefix")
elif not is_system_path(self.spec["iconv"].prefix):
args.append("--with-libiconv-prefix=" + self.spec["iconv"].prefix)

# Renamed or modified flags
if self.spec.satisfies("@3:"):
Expand Down
7 changes: 6 additions & 1 deletion var/spack/repos/builtin/packages/gettext/package.py
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
import re

from spack.package import *
from spack.util.environment import is_system_path


class Gettext(AutotoolsPackage, GNUMirrorPackage):
Expand Down Expand Up @@ -78,7 +79,6 @@ def configure_args(self):
config_args = [
"--disable-java",
"--disable-csharp",
"--with-libiconv-prefix={0}".format(spec["iconv"].prefix),
"--with-included-glib",
"--with-included-gettext",
"--with-included-libcroco",
Expand All @@ -87,6 +87,11 @@ def configure_args(self):
"--without-cvs",
]

if self.spec["iconv"].name == "libc":
config_args.append("--without-libiconv-prefix")
elif not is_system_path(self.spec["iconv"].prefix):
config_args.append("--with-libiconv-prefix=" + self.spec["iconv"].prefix)

if "+curses" in spec:
config_args.append("--with-ncurses-prefix={0}".format(spec["ncurses"].prefix))
else:
Expand Down
27 changes: 20 additions & 7 deletions var/spack/repos/builtin/packages/git/package.py
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@
import re

from spack.package import *
from spack.util.environment import is_system_path


class Git(AutotoolsPackage):
Expand Down Expand Up @@ -466,12 +467,18 @@ def setup_build_environment(self, env):
# The test avoids failures when git is an external package.
# In that case the node in the DAG gets truncated and git DOES NOT
# have a gettext dependency.
if "+nls" in self.spec:
if "intl" in self.spec["gettext"].libs.names:
env.append_flags("EXTLIBS", "-L{0} -lintl".format(self.spec["gettext"].prefix.lib))
env.append_flags("CFLAGS", "-I{0}".format(self.spec["gettext"].prefix.include))

if "~perl" in self.spec:
spec = self.spec
if "+nls" in spec:
if "intl" in spec["gettext"].libs.names:
extlib_bits = []
if not is_system_path(spec["gettext"].prefix):
extlib_bits.append(spec["gettext"].libs.search_flags)
extlib_bits.append("-lintl")
env.append_flags("EXTLIBS", " ".join(extlib_bits))
if not is_system_path(spec["gettext"].prefix):
env.append_flags("CFLAGS", spec["gettext"].headers.include_flags)
Copy link
Member

Choose a reason for hiding this comment

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

This drops optimization flags :(


if "~perl" in spec:
env.append_flags("NO_PERL", "1")

def configure_args(self):
Expand All @@ -480,11 +487,17 @@ def configure_args(self):
configure_args = [
"--with-curl={0}".format(spec["curl"].prefix),
"--with-expat={0}".format(spec["expat"].prefix),
"--with-iconv={0}".format(spec["iconv"].prefix),
"--with-openssl={0}".format(spec["openssl"].prefix),
"--with-zlib={0}".format(spec["zlib"].prefix),
]

if not self.spec["iconv"].name == "libc":
configure_args.append(
"--with-iconv={0}".format(
"yes" if is_system_path(spec["iconv"].prefix) else spec["iconv"].prefix
)
)

if "+perl" in self.spec:
configure_args.append("--with-perl={0}".format(spec["perl"].command.path))

Expand Down
13 changes: 9 additions & 4 deletions var/spack/repos/builtin/packages/glib/package.py
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
import os.path

from spack.package import *
from spack.util.environment import is_system_path


class Glib(Package):
Expand Down Expand Up @@ -210,7 +211,7 @@ def meson_args(self):
if self.spec.satisfies("@:2.72"):
args.append("-Dgettext=external")
if self.spec.satisfies("@:2.74"):
if "libc" in self.spec:
if self.spec["iconv"].name == "libc":
args.append("-Diconv=libc")
else:
if self.spec.satisfies("@2.61.0:"):
Expand Down Expand Up @@ -244,7 +245,7 @@ def configure_args(self):
args.append(
"--with-python={0}".format(os.path.basename(self.spec["python"].command.path))
)
if "libc" in self.spec:
if self.spec["iconv"].name == "libc":
args.append("--with-libiconv=maybe")
else:
args.append("--with-libiconv=gnu")
Expand Down Expand Up @@ -350,10 +351,14 @@ def gettext_libdir(self):
# Packages that link to glib were also picking up -lintl from glib's
# glib-2.0.pc file. However, packages such as py-pygobject were
# bypassing spack's compiler wrapper for linking and thus not finding
# the gettext library directory. The patch below explitly adds the
# the gettext library directory. The patch below explicitly adds the
# appropriate -L path.
spec = self.spec
if spec.satisfies("@2.0:2"):
if (
spec.satisfies("@2.0:2")
and "intl" in self.spec["gettext"].libs.names
and not is_system_path(spec["gettext"].prefix)
):
pattern = "Libs:"
repl = "Libs: -L{0} -Wl,-rpath={0} ".format(spec["gettext"].libs.directories[0])
myfile = join_path(self.spec["glib"].libs.directories[0], "pkgconfig", "glib-2.0.pc")
Expand Down
7 changes: 5 additions & 2 deletions var/spack/repos/builtin/packages/gnupg/package.py
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

from spack.package import *
from spack.util.environment import is_system_path


class Gnupg(AutotoolsPackage):
Expand Down Expand Up @@ -142,7 +143,6 @@ def configure_args(self):
"--disable-regex",
"--with-zlib=" + self.spec["zlib"].prefix,
"--without-tar",
"--without-libiconv-prefix",
"--without-readline",
]

Expand All @@ -158,9 +158,12 @@ def configure_args(self):
"--with-libassuan-prefix=" + self.spec["libassuan"].prefix,
"--with-ksba-prefix=" + self.spec["libksba"].prefix,
"--with-npth-prefix=" + self.spec["npth"].prefix,
"--with-libiconv-prefix=" + self.spec["iconv"].prefix,
]
)
if self.spec["iconv"].name == "libc":
args.append("--without-libiconv-prefix")
elif not is_system_path(self.spec["iconv"].prefix):
args.append("--with-libiconv-prefix=" + self.spec["iconv"].prefix)

if self.spec.satisfies("@:1"):
args.extend(
Expand Down