Add mergeback for -D and -W cflags to match pkgconf behavior #43

otegami · 2025-12-01T03:58:10Z

When multiple packages have the same -D or -W flags, pkgconf removes previous occurrences and keeps only the last one (merge back).
This prevents flag duplication like "-DNOMINMAX"
appearing many times.

$ pkgconf --cflags re2
-pthread -DNOMINMAX
$ ruby -r pkg-config -e 'puts PKGConfig.cflags("re2")'
-pthread -DNOMINMAX -DNOMINMAX -DNOMINMAX ...

Flags excluded from merge back (kept as duplicates):

-Wa, (assembler options)
-Wl, (linker options)
-Wp, (preprocessor options)

This PR only supports merge back for -D and -W flags. Other flags that pkgconf also merges back are not yet supported.

When multiple packages have the same -D or -W flags, pkgconf removes previous occurrences and keeps only the last one (mergeback). This prevents flag duplication like "-DNOMINMAX" appearing many times. ```console$ pkgconf --cflags re2 -pthread -DNOMINMAX $ ruby -r pkg-config -e 'puts PKGConfig.cflags("re2")' -pthread -DNOMINMAX -DNOMINMAX -DNOMINMAX ... ``` Flags excluded from mergeback (kept as duplicates): - -Wa, (assembler options) - -Wl, (linker options) - -Wp, (preprocessor options) This PR only supports mergeback for -D and -W flags. Other flags that pkgconf also mergebacks are not yet supported.

kou · 2025-12-01T04:39:31Z

lib/pkg-config.rb

    normalized_cflags
  end

+  def mergeback_flags(flags)


Could you add a comment that we want to implement pkgconf's pkgconf_fragment_copy() compatible behavior here?

https://github.com/pkgconf/pkgconf/blob/pkgconf-2.5.1/libpkgconf/fragment.c#L381-L416

fix: 19cd8b8

kou · 2025-12-01T04:51:39Z

lib/pkg-config.rb

+    mergebacked_flags = []
+    flags.each do |flag|
+      if mergeable_flag?(flag)
+        mergebacked_flags.delete(flag)


Could you add a comment that this may be slow because this checks mergebacked_flags N (the number of mergeable flags) times?

fix: de47ab7

kou · 2025-12-01T04:52:02Z

lib/pkg-config.rb

        flag.gsub(/\A-I/, "/I")
      end
    end
+    other_flags = mergeback_flags(other_flags)


Could you use merge_back not mergeback?

Could you use cflags not flags? We only support C flags for now, right?

fix: c6b9da8

kou · 2025-12-01T04:57:34Z

test/test-pkg-config.rb

    end
  end
+
+  sub_test_case("cflags mergeback") do


Can we simplify this?

sub_test_case("#merge_back_cflags") do def merge_back_cflags(cflags) @glib.__send__(:merge_back_cflags, cflags) end def test_define assert_equal("-I/main -I/dep -DFOO", merge_back_cflags("-I/main -DFOO -I/dep -DFOO")) end end

See also:

pkg-config/test/test-pkg-config.rb

Lines 282 to 316 in ca9ad8a

sub_test_case("#parse_requires") do

def parse_requires(requires)

@glib.__send__(:parse_requires, requires)

end

def test_broken_version

assert_equal(["fribidi"],

parse_requires("fribidi >= fribidi_required_dep"))

end

def test_greater_than_or_equals_to

assert_equal(["fribidi"],

parse_requires("fribidi >= 1.0"))

end

def test_greater_than

assert_equal(["fribidi"],

parse_requires("fribidi > 1.0"))

end

def test_less_than_or_equals_to

assert_equal(["fribidi"],

parse_requires("fribidi <= 1.0"))

end

def test_less_than

assert_equal(["fribidi"],

parse_requires("fribidi < 1.0"))

end

def test_equals_to

assert_equal(["fribidi"],

parse_requires("fribidi = 1.0"))

end

end

fix: 062f4be

otegami · 2025-12-01T07:11:36Z

Thank you for reviews. I've just addressed all of review comments, so far.

kou · 2025-12-01T07:19:20Z

lib/pkg-config.rb

    end
    all_cflags = normalize_cflags(Shellwords.split(cflags_set.join(" ")))
-    path_flags, other_flags = all_cflags.partition {|flag| /\A-I/ =~ flag}
+    path_flags, cflags = all_cflags.partition {|flag| /\A-I/ =~ flag}


We can keep other_flags because -I is also C flags and this method is collect_cflags.

fix: 5230529 Sure.

It's because this method collect cflags, so all of them are cflags too.

kou · 2025-12-01T07:20:13Z

lib/pkg-config.rb

+  # NOTE: This may be slow because this checks merge_back_cflags N times (where
+  # N is the number of mergeable flags).


Could you move this to just before mergebacked_cflags.delete(cflag)?
The code may be slow.

fix: b9a7cdc

kou · 2025-12-01T07:20:26Z

lib/pkg-config.rb

+  # NOTE: This may be slow because this checks merge_back_cflags N times (where
+  # N is the number of mergeable flags).
+  def merge_back_cflags(cflags)
+    mergebacked_cflags = []


Suggested change

mergebacked_cflags = []

merge_backed_cflags = []

fix: 6e7d245

kou · 2025-12-01T07:22:06Z

test/test-pkg-config.rb

+                                      "-Wa,--noexecstack", "-Wl,--as-needed", "-Wp,-DFOO"]))
+    end
+
+    def test_mixed_flags


Suggested change

def test_mixed_flags

def test_mixed

fix: c21530f

test/test-pkg-config.rb

otegami · 2025-12-01T07:36:05Z

Thank you for reviews. I've just addressed all of review comments, so far.

kou · 2025-12-01T07:56:07Z

Update the PR description before I merge this.

kou reviewed Dec 1, 2025

View reviewed changes

otegami added 4 commits December 1, 2025 15:56

Added the comment about the method's specification

19cd8b8

Add the note about the speed of the function

de47ab7

Manipulate the only cflags here

c6b9da8

Simplify the test cases

062f4be

otegami requested a review from kou December 1, 2025 07:11

kou reviewed Dec 1, 2025

View reviewed changes

Renamed the variable name to other_flags

5230529

It's because this method collect cflags, so all of them are cflags too.

kou reviewed Dec 1, 2025

View reviewed changes

otegami added 3 commits December 1, 2025 16:31

Moved the comment to the appropriate place

b9a7cdc

Fixed the typo

6e7d245

Remove the redundatant words from tests

c21530f

otegami requested a review from kou December 1, 2025 07:36

kou merged commit 7caeccc into ruby-gnome:main Dec 1, 2025
21 checks passed

otegami deleted the mergeback-d-and-w-flags branch December 1, 2025 07:56

	sub_test_case("#parse_requires") do
	def parse_requires(requires)
	@glib.__send__(:parse_requires, requires)
	end

	def test_broken_version
	assert_equal(["fribidi"],
	parse_requires("fribidi >= fribidi_required_dep"))
	end

	def test_greater_than_or_equals_to
	assert_equal(["fribidi"],
	parse_requires("fribidi >= 1.0"))
	end

	def test_greater_than
	assert_equal(["fribidi"],
	parse_requires("fribidi > 1.0"))
	end

	def test_less_than_or_equals_to
	assert_equal(["fribidi"],
	parse_requires("fribidi <= 1.0"))
	end

	def test_less_than
	assert_equal(["fribidi"],
	parse_requires("fribidi < 1.0"))
	end

	def test_equals_to
	assert_equal(["fribidi"],
	parse_requires("fribidi = 1.0"))
	end
	end

		# NOTE: This may be slow because this checks merge_back_cflags N times (where
		# N is the number of mergeable flags).

Add mergeback for -D and -W cflags to match pkgconf behavior #43

Add mergeback for -D and -W cflags to match pkgconf behavior #43

Uh oh!

Conversation

otegami commented Dec 1, 2025 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otegami commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

otegami commented Dec 1, 2025

Uh oh!

kou commented Dec 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

otegami commented Dec 1, 2025 •

edited by kou

Loading