extract lib_path and include_path methods

also: - reduce the number of conditional checks in `mkmf_config` - prefer "shellsplit" to "split" when breaking up command flags - prepend to `$libs` instead of appending to it
flavorjones · Sep 17, 2023 · 6b61ce6 · 6b61ce6
1 parent c3a27ed
commit 6b61ce6
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 44 deletions.
diff --git a/lib/mini_portile2/mini_portile.rb b/lib/mini_portile2/mini_portile.rb
@@ -237,11 +237,10 @@ def cook
   end
 
   def activate
-    lib_path = File.join(port_path, "lib")
     vars = {
       'PATH'          => File.join(port_path, 'bin'),
-      'CPATH'         => File.join(port_path, 'include'),
-      'LIBRARY_PATH'  => lib_path
+      'CPATH'         => include_path,
+      'LIBRARY_PATH'  => lib_path,
     }.reject { |env, path| !File.directory?(path) }
 
     output "Activating #{@name} #{@version} (from #{port_path})..."
@@ -272,7 +271,7 @@ def mkmf_config(pkg: nil, dir: nil)
     require "mkmf"
 
     if pkg
-      dir ||= File.join(path, "lib", "pkgconfig")
+      dir ||= File.join(lib_path, "pkgconfig")
       pcfile = File.join(dir, "#{pkg}.pc")
       unless File.exist?(pcfile)
         raise ArgumentError, "pkg-config file '#{pcfile}' does not exist"
@@ -293,37 +292,40 @@ def mkmf_config(pkg: nil, dir: nil)
     else
       output "Configuring MakeMakefile for #{@name} #{@version} (from #{path})\n"
 
-      include_path = File.join(path, "include")
-      lib_path = File.join(path, "lib")
-
       lib_name = name.sub(/\Alib/, "") # TODO: use delete_prefix when we no longer support ruby 2.4
 
-      incflags = "-I#{include_path}" if Dir.exist?(include_path)
-      ldflags = "-L#{lib_path}" if Dir.exist?(lib_path)
-      libflags = "-l#{lib_name}" if Dir.exist?(lib_path)
-    end
-
-    if ldflags
-      libpaths = ldflags.split.map { |f| f.sub(/\A-L/, "") }
+      incflags = Dir.exist?(include_path) ? "-I#{include_path}" : ""
+      cflags = ""
+      ldflags = Dir.exist?(lib_path) ? "-L#{lib_path}" : ""
+      libflags = Dir.exist?(lib_path) ? "-l#{lib_name}" : ""
     end
 
-    # prefer this package by prepending directories to the search path
+    # prefer this package by prepending to search paths and library flags
     #
-    # use $LIBPATH instead of $LDFLAGS to ensure we get the `-Wl,-rpath` linker flag for re-finding
-    # shared libraries
-    $INCFLAGS = [incflags, $INCFLAGS].join(" ").strip if incflags
-    $LIBPATH = libpaths | $LIBPATH if libpaths
+    # convert the ldflags into a list of directories and append to $LIBPATH (instead of just using
+    # $LDFLAGS) to ensure we get the `-Wl,-rpath` linker flag for re-finding shared libraries.
+    $INCFLAGS = [incflags, $INCFLAGS].join(" ").strip
+    libpaths = ldflags.shellsplit.map { |f| f.sub(/\A-L/, "") }
+    $LIBPATH = libpaths | $LIBPATH
+    $libs = [libflags, $libs].join(" ").strip
 
-    # prefer this package's flags by appending them to the command line
-    $CFLAGS = [$CFLAGS, cflags].join(" ").strip if cflags
-    $CXXFLAGS = [$CXXFLAGS, cflags].join(" ").strip if cflags
-    $libs = [$libs, libflags].join(" ").strip if libflags
+    # prefer this package's compiler flags by appending them to the command line
+    $CFLAGS = [$CFLAGS, cflags].join(" ").strip
+    $CXXFLAGS = [$CXXFLAGS, cflags].join(" ").strip
   end
 
   def path
     File.expand_path(port_path)
   end
 
+  def include_path
+    File.join(path, "include")
+  end
+
+  def lib_path
+    File.join(path, "lib")
+  end
+
   def gcc_cmd
     (ENV["CC"] || @gcc_command || RbConfig::CONFIG["CC"] || "gcc").dup
   end

diff --git a/test/test_mkmf_config.rb b/test/test_mkmf_config.rb
@@ -3,7 +3,7 @@
 require "mkmf" # initialize $LDFLAGS et al here, instead of in the middle of a test
 
 class TestMkmfConfig < TestCase
-  attr_reader :recipe, :include_path, :lib_path
+  attr_reader :recipe
 
   LIBXML_PCP = File.join(__dir__, "assets", "pkgconf", "libxml2")
   LIBXSLT_PCP = File.join(__dir__, "assets", "pkgconf", "libxslt")
@@ -25,8 +25,6 @@ def setup
     @recipe = MiniPortile.new("libfoo", "1.0.0").tap do |recipe|
       recipe.logger = StringIO.new
     end
-    @include_path = File.join(@recipe.path, "include")
-    @lib_path = File.join(@recipe.path, "lib")
   end
 
   def teardown
@@ -47,35 +45,35 @@ def teardown
   def test_mkmf_config_recipe_LIBPATH_global_lib_dir_does_not_exist
     recipe.mkmf_config
 
-    refute_includes($LIBPATH, lib_path)
-    refute_includes($libs.split, "-lfoo")
+    refute_includes($LIBPATH, recipe.lib_path)
+    refute_includes($libs.shellsplit, "-lfoo")
   end
 
   def test_mkmf_config_recipe_LIBPATH_global
-    FileUtils.mkdir_p(lib_path)
+    FileUtils.mkdir_p(recipe.lib_path)
 
     recipe.mkmf_config
 
-    assert_includes($LIBPATH, lib_path)
-    assert_operator($LIBPATH.index(lib_path), :<, $LIBPATH.index("xxx")) # prepend
+    assert_includes($LIBPATH, recipe.lib_path)
+    assert_operator($LIBPATH.index(recipe.lib_path), :<, $LIBPATH.index("xxx")) # prepend
 
-    assert_includes($libs.split, "-lfoo") # note the recipe name is "libfoo"
-    assert_match(%r{-lxxx.*-lfoo}, $libs) # append
+    assert_includes($libs.shellsplit, "-lfoo") # note the recipe name is "libfoo"
+    assert_match(%r{-lfoo.*-lxxx}, $libs) # prepend
   end
 
   def test_mkmf_config_recipe_INCFLAGS_global_include_dir_does_not_exist
     recipe.mkmf_config
 
-    refute_includes($INCFLAGS.split, "-I#{include_path}")
+    refute_includes($INCFLAGS.shellsplit, "-I#{recipe.include_path}")
   end
 
   def test_mkmf_config_recipe_INCFLAGS_global
-    FileUtils.mkdir_p(include_path)
+    FileUtils.mkdir_p(recipe.include_path)
 
     recipe.mkmf_config
 
-    assert_includes($INCFLAGS.split, "-I#{include_path}")
-    assert_match(%r{-I#{include_path}.*-I/xxx}, $INCFLAGS) # prepend
+    assert_includes($INCFLAGS.shellsplit, "-I#{recipe.include_path}")
+    assert_match(%r{-I#{recipe.include_path}.*-I/xxx}, $INCFLAGS) # prepend
   end
 
   def test_mkmf_config_pkgconf_does_not_exist
@@ -93,8 +91,8 @@ def test_mkmf_config_pkgconf_LIBPATH_global
     assert_includes($LIBPATH, "/foo/libxml2/2.11.5/lib")
     assert_operator($LIBPATH.index("/foo/libxml2/2.11.5/lib"), :<, $LIBPATH.index("xxx")) # prepend
 
-    assert_includes($libs.split, "-lxml2")
-    assert_match(%r{-lxxx.*-lxml2}, $libs) # append
+    assert_includes($libs.shellsplit, "-lxml2")
+    assert_match(%r{-lxml2.*-lxxx}, $libs) # prepend
   end
 
   def test_mkmf_config_pkgconf_CFLAGS_global
@@ -103,14 +101,14 @@ def test_mkmf_config_pkgconf_CFLAGS_global
 
     recipe.mkmf_config(pkg: "libxml-2.0", dir: LIBXML_PCP)
 
-    assert_includes($INCFLAGS.split, "-I/foo/libxml2/2.11.5/include/libxml2")
+    assert_includes($INCFLAGS.shellsplit, "-I/foo/libxml2/2.11.5/include/libxml2")
     assert_match(%r{-I/foo/libxml2/2.11.5/include/libxml2.*-I/xxx}, $INCFLAGS) # prepend
 
-    assert_includes($CFLAGS.split, "-ggdb3")
-    assert_match(%r{-xxx.*-ggdb3}, $CFLAGS) # prepend
+    assert_includes($CFLAGS.shellsplit, "-ggdb3")
+    assert_match(%r{-xxx.*-ggdb3}, $CFLAGS) # append
 
-    assert_includes($CXXFLAGS.split, "-ggdb3")
-    assert_match(%r{-xxx.*-ggdb3}, $CXXFLAGS) # prepend
+    assert_includes($CXXFLAGS.shellsplit, "-ggdb3")
+    assert_match(%r{-xxx.*-ggdb3}, $CXXFLAGS) # append
   end
 
   def test_mkmf_config_pkgconf_path_accumulation

diff --git a/test/test_recipe.rb b/test/test_recipe.rb
@@ -0,0 +1,18 @@
+require File.expand_path('../helper', __FILE__)
+
+class TestRecipe < TestCase
+  def test_path
+    recipe = MiniPortile.new("libfoo", "1.0.0")
+    assert_equal(File.expand_path(File.join(recipe.target, recipe.host, recipe.name, recipe.version)), recipe.path)
+  end
+
+  def test_lib_path
+    recipe = MiniPortile.new("libfoo", "1.0.0")
+    assert_equal(File.join(recipe.path, "lib"), recipe.lib_path)
+  end
+
+  def test_include_path
+    recipe = MiniPortile.new("libfoo", "1.0.0")
+    assert_equal(File.join(recipe.path, "include"), recipe.include_path)
+  end
+end