Add flags to superenv when building with [email protected] #13568

danielnachun · 2022-07-17T02:58:22Z

This PR should fix most of the issues we were hitting in Homebrew/homebrew-portable-ruby#156. The root of the problem is that we can't mix headers from the system glibc with [email protected] and the only way to guarantee this is to use -nostdinc and add the paths to the GCC headers with -isystem flags.

BrewTestBot · 2022-07-17T02:58:36Z

Review period will end on 2022-07-19 at 00:00:00 UTC.

Bo98 · 2022-07-17T06:18:28Z

Library/Homebrew/extend/os/linux/extend/ENV/super.rb

+      gcc_include_dir, = Open3.capture2("#{cc} --print-file-name=include")
+      gcc_include_fixed_dir, = Open3.capture2("#{cc} --print-file-name=include-fixed")
+      paths << gcc_include_dir.strip << gcc_include_fixed_dir.strip


Suggested change

gcc_include_dir, = Open3.capture2("#{cc} --print-file-name=include")

gcc_include_fixed_dir, = Open3.capture2("#{cc} --print-file-name=include-fixed")

paths << gcc_include_dir.strip << gcc_include_fixed_dir.strip

gcc_include_dir = Utils.safe_popen_read(cc, "--print-file-name=include").chomp

gcc_include_fixed_dir = Utils.safe_popen_read(cc, "--print-file-name=include-fixed").chomp

paths << gcc_include_dir << gcc_include_fixed_dir

This is more consistent with what we do elsewhere, assuming this works.

I had a feeling that Open3 wasn't what we normally used. I'll test this in the Docker image before pushing to here.

danielnachun · 2022-07-17T23:18:31Z

I'm marking this as a draft for now because I need to push commits to this that I can test in a VM but I don't want it to be merged until the testing is done.

danielnachun · 2022-07-18T00:17:15Z

There was just one minor issue I had to fix and now portable-ruby is building as expected.

MikeMcQuaid

Makes sense to me!

BrewTestBot · 2022-07-18T08:10:20Z

Review period skipped due to critical label.

Library/Homebrew/extend/os/linux/extend/ENV/super.rb

danielnachun · 2022-07-18T18:28:23Z

I know @Bo98 is busy with the ephemeral CI stuff right now but I would probably be good for him to have one more chance to look at this before merging given that this is tightly integrated with Homebrew/homebrew-portable-ruby#156.

Bo98 · 2022-07-19T14:19:10Z

Library/Homebrew/shims/super/cc

@@ -326,6 +328,9 @@ class Cmd
    end
    args += rpath_flags("#{wl}-rpath=", rpath_paths)
    args += ["#{wl}--dynamic-linker=#{dynamic_linker_path}"] if dynamic_linker_path
+    # Use -rpath-link to make sure linker uses [email protected] rather than the system glibc for indirect
+    # dependencies because -L will only handle direct dependencies.
+    args << "#{wl}-rpath-link=#{@opt}/[email protected]/lib" if @deps.include?("[email protected]")


Do we need to change the -B a few lines above?

I guess it would only matter if the user was building against [email protected] on a host where brewed glibc was also installed. Given that we're only building against [email protected] for relocatable bottles I don't think scenario should happen very often. We could probably accommodate this by just doing:

if @deps.include?("[email protected]") args << "-B#{@opt}/[email protected]/lib" else args << "-B#{@opt}/glibc/lib" end

If this sounds good I'll push the change.

Did you decide against pushing this?

I suspect @danielnachun forgot.

Sorry I misunderstood your comment as wanting to merge as-is. We need to fix the issue below with unconditionally passing nostdinc so I will add this in a follow up PR.

Bo98 · 2022-07-20T13:48:39Z

Library/Homebrew/shims/super/cc

@@ -292,6 +292,8 @@ class Cmd
    args.concat(optflags) unless runtime_cpu_detection?
    args.concat(archflags)
    args << "-std=#{@arg0}" if /c[89]9/.match?(@arg0)
+    # Add -nostdinc when building against [email protected] to avoid mixing system and brewed glibc headers.
+    args << "-nostdinc" if @deps.include?("[email protected]")


Looks like this is gated to not run if we're not doing arg refurbishment (line 287).

I argue we actually want to do this unconditionally. I think this should be in cppflags instead, like the other -isystem flags. cppflags applies to all modes - including cpp invokations where neither cflags nor cxxflags are used.

That makes sense. I tried this and it fixed the issue where the system headers were still being included seen here: Homebrew/homebrew-portable-ruby#156 (comment).

MikeMcQuaid · 2022-07-21T09:36:55Z

Library/Homebrew/extend/os/linux/extend/ENV/super.rb

+  def homebrew_extra_isystem_paths
+    paths = []
+    # Add paths for GCC headers when building against [email protected] because we have to use -nostdinc.
+    if deps.any? { |d| d.name == "[email protected]" }


@danielnachun I'd really like if we can extract all these references into a global constant or, if that doesn't allow superenv handling, a file somewhere.

So basically use a variable instead of hard coding [email protected] so we only have to change it once? That does seem better for maintainability, I just have to see where I can define it that will be seen by both the superenv and the compiler shim.

@danielnachun I think the answer may be "nowhere" but at least if it's in two places with a comment in both that says to change them both at once that'd be better!

Our compiler shim doesn't load the Homebrew module, but we can definitely put this into a global constant which is passed to the compiler shim via an environment variable.

ooo environment variable, good idea 👍🏻

Or the existing cccfg system

I think we could append_to_cccfg a character for whenever glibc@#{MIN_GLIBC_VERSION} is a dependency where MIN_GLIBC_VERSION is an environment variable we set once globally. The individual conditionals on [email protected] being a dependency could then be replaced with conditional on HOMEBREW_CCCFG containing the relevant character, and the actual flags we set would also use #{MIN_GLIBC_VERSION} instead of the hardcoded 2.13 we're currently using. That should mean that whenever the version needs to be changed, we can just update MIN_GLIBC_VERSION and everything else will be updated automatically because it's not hardcoded.

Now that we know our strategy for handling [email protected] works I can just test the refactoring by trying to build binutils as it will fail quickly if I mess something up. Thanks for the great suggestions for this!

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 17, 2022

danielnachun requested a review from Bo98 July 17, 2022 02:58

danielnachun mentioned this pull request Jul 17, 2022

portable_formula: use [email protected] on Linux Homebrew/homebrew-portable-ruby#156

Merged

Bo98 reviewed Jul 17, 2022

View reviewed changes

danielnachun marked this pull request as draft July 17, 2022 23:17

danielnachun force-pushed the fix_glibc_2_13 branch 2 times, most recently from 82059ce to 989d8c1 Compare July 17, 2022 23:24

danielnachun added 2 commits July 17, 2022 17:15

shims/super/cc: add nostdinc and rpath-link when using [email protected]

6ab6382

extend/os/linux/extend/ENV/super.rb: add homebrew_extra_include_paths

47b6492

danielnachun force-pushed the fix_glibc_2_13 branch from 989d8c1 to 47b6492 Compare July 18, 2022 00:15

danielnachun marked this pull request as ready for review July 18, 2022 00:16

danielnachun requested a review from Bo98 July 18, 2022 00:17

MikeMcQuaid approved these changes Jul 18, 2022

View reviewed changes

MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Jul 18, 2022

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 18, 2022

BrewTestBot approved these changes Jul 18, 2022

View reviewed changes

carlocab reviewed Jul 18, 2022

View reviewed changes

Library/Homebrew/extend/os/linux/extend/ENV/super.rb Show resolved Hide resolved

Bo98 reviewed Jul 19, 2022

View reviewed changes

danielnachun merged commit 8a2efb1 into Homebrew:master Jul 20, 2022

Bo98 reviewed Jul 20, 2022

View reviewed changes

danielnachun mentioned this pull request Jul 21, 2022

binutils: remove unneeded CFLAGS Homebrew/homebrew-core#106279

Closed

6 tasks

MikeMcQuaid reviewed Jul 21, 2022

View reviewed changes

github-actions bot added the outdated PR was locked due to age label Aug 21, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2022

danielnachun deleted the fix_glibc_2_13 branch September 2, 2022 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flags to superenv when building with [email protected] #13568

Add flags to superenv when building with [email protected] #13568

danielnachun commented Jul 17, 2022

BrewTestBot commented Jul 17, 2022

Bo98 Jul 17, 2022 •

edited

Loading

danielnachun Jul 17, 2022

danielnachun commented Jul 17, 2022

danielnachun commented Jul 18, 2022

MikeMcQuaid left a comment

BrewTestBot commented Jul 18, 2022

danielnachun commented Jul 18, 2022

Bo98 Jul 19, 2022

danielnachun Jul 19, 2022

Bo98 Jul 19, 2022

Bo98 Jul 20, 2022

carlocab Jul 20, 2022

danielnachun Jul 20, 2022

Bo98 Jul 20, 2022 •

edited

Loading

danielnachun Jul 20, 2022

MikeMcQuaid Jul 21, 2022

danielnachun Jul 21, 2022

MikeMcQuaid Jul 21, 2022

carlocab Jul 21, 2022 •

edited

Loading

MikeMcQuaid Jul 21, 2022

Bo98 Jul 21, 2022

danielnachun Jul 21, 2022

Add flags to superenv when building with [email protected] #13568

Add flags to superenv when building with [email protected] #13568

Conversation

danielnachun commented Jul 17, 2022

BrewTestBot commented Jul 17, 2022

Bo98 Jul 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnachun commented Jul 17, 2022

danielnachun commented Jul 18, 2022

MikeMcQuaid left a comment

Choose a reason for hiding this comment

BrewTestBot commented Jul 18, 2022

danielnachun commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bo98 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bo98 Jul 17, 2022 •

edited

Loading

Bo98 Jul 20, 2022 •

edited

Loading

carlocab Jul 21, 2022 •

edited

Loading