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.

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

formulae: stricter linkage test. #1059

Closed
wants to merge 1 commit into from
Closed

Conversation

MikeMcQuaid
Copy link
Member

Check for indirect dependencies with linkage and undeclared dependencies with linkage in test mode.

This should be done to ensure we accurately declare dependencies in homebrew/core.

Alternate/complementary approach in Homebrew/brew#17286

Check for indirect dependencies with linkage and undeclared dependencies
with linkage in test mode.

This should be done to ensure we accurately declare dependencies in
homebrew/core.
@@ -530,18 +530,10 @@ def formula!(formula_name, args:)
bottle_reinstall_formula(formula, new_formula, args:)
end
@built_formulae << formula.full_name
test("brew", "linkage", "--test", named_args: formula_name, ignore_failures:, report_analytics: true)
test("brew", "linkage", "--test", "--strict", named_args: formula_name, ignore_failures:, report_analytics: true)
Copy link
Member

Choose a reason for hiding this comment

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

This will break gcc and julia builds. Haven't really been able to work out how to prevent their failures under --strict (because they have files with missing rpaths). The warning is, AFAICT, harmless for julia, but not necessarily for GCC (though it happens for a compatibility library that is either deprecated or about to be).

CC @fxcoudert for thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

For compiler runtime libraries, the lack of rpaths is not necessarily an issue. For GCC, the compatibility library is not going away, but the lack of rpath (a historical feature) is not a problem since it is only supposed to be linked by gcc itself, which will add the correct path in the executable.

I think the solution is to differentiate between different audits:

  • lack of rpath in some cases is not a fatal flaw, and should not be an error
  • linking to a library not in a dependency should be an error (in strict mode)

Copy link
Member

Choose a reason for hiding this comment

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

For GCC, the compatibility library is not going away, but the lack of rpath (a historical feature) is not a problem since it is only supposed to be linked by gcc itself, which will add the correct path in the executable.

Sadly it is a problem for us, because GCC adds LC_RPATH commands referencing Cellar paths to the binaries it links, so we need the compatibility library to have a correct (relative) LC_RPATH command.

Copy link
Member Author

Choose a reason for hiding this comment

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

CC @Bo98 too.

What is the proposed solution here? I see a few:

  1. don't do this
  2. do this and have an allowlist for gcc/julia in Homebrew/brew
  3. do this and have an allowlist for gcc/julia in Homebrew/homebrew-test-bot
  4. do this but with something else too

Copy link
Member

@Bo98 Bo98 May 14, 2024

Choose a reason for hiding this comment

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

Almost feels like we maybe need a middle ground:

  • --test that brew install runs`
  • --test --??? that brew test-bot runs (but not on dependents)
  • --test --strict for warnings

However, more generally: I don't consider brew linkage false positives acceptable even with an allowlist. Either the gcc/julia is wrong or the rpath check is wrong. Past attempts to add allowlists to brew linkage have all ended up being removed for being wrong (see ignore_missing_libraries 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.

Ok, thanks. Closing because it's not clear what needs to be done here.

Copy link
Member

Choose a reason for hiding this comment

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

Here are our options to unblock this change:

  1. Fix GCC and Julia so that they don't fail brew linkage --test --strict.
  2. Create an allowlist for GCC and Julia that allows us to ignore brew linkage --test --strict failures.
  3. Add a new flag to brew linkage that enables making indirect linkage an error in test-bot.
  4. Get rid of the missing RPATH check in brew linkage.

(1) can be done easily for GCC with either a slightly hacky solution (adding RPATHs), or with rm lib/"libgcc_s.1.dylib". It is likely to be fragile for Julia.

(2) is subject to @Bo98's criticism above:

However, more generally: I don't consider brew linkage false positives acceptable even with an allowlist. Either the gcc/julia is wrong or the rpath check is wrong. Past attempts to add allowlists to brew linkage have all ended up being removed for being wrong (see ignore_missing_libraries stuff).

(For clarity: I think gcc/julia is wrong, and have explained why above, but I believe FX still disagrees with me.)

(3) could make the flags for brew linkage confusing/complicated, if we went down a --strict and --stricter route for the flags. It might be more viable if we did --strict, which enables all strict checks, and --strict=foo,bar, where foo and bar are the strict checks we want to run (e.g. --strict=indirect-linkage).

(4) would be easy, but it limits our ability to proactively fix issues that could be a problem for users.

Copy link
Member

Choose a reason for hiding this comment

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

  • For GCC, removing the compat library is wrong, but fixing its rpath is acceptable. I don't think it's a bug and the audit is being overly picky, but it's not a big deal.
  • Re. Julia, has an issue been opened upstream?

Copy link
Member

Choose a reason for hiding this comment

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

Why isn't the fact that user binaries that link to libgcc_s.1 will break when the GCC pkg_version changes a bug?

I'm also sceptical that the lack of an LC_RPATH command in libgcc_s.1 is deliberate. From the libraries in GCC, here are the ones with an @rpath reference:

❯ fd --type=file --extension=dylib -x sh -c 'otool -L {} | rg -q rpath && echo {}' | xargs otool -L
./libgcc_s.1.dylib:
        /usr/local/opt/gcc/lib/gcc/current/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.1.0)
        @rpath/libgcc_s.1.1.dylib (compatibility version 1.0.0, current version 1.1.0, reexport)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
./libobjc-gnu.4.dylib:
        /usr/local/opt/gcc/lib/gcc/current/libobjc-gnu.4.dylib (compatibility version 5.0.0, current version 5.0.0)
        @rpath/libgcc_s.1.1.dylib (compatibility version 1.0.0, current version 1.1.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
./libgfortran.5.dylib:
        /usr/local/opt/gcc/lib/gcc/current/libgfortran.5.dylib (compatibility version 6.0.0, current version 6.0.0)
        @rpath/libquadmath.0.dylib (compatibility version 1.0.0, current version 1.0.0)
        @rpath/libgcc_s.1.1.dylib (compatibility version 1.0.0, current version 1.1.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
./libstdc++.6.dylib:
        /usr/local/opt/gcc/lib/gcc/current/libstdc++.6.dylib (compatibility version 7.0.0, current version 7.32.0)
        /usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
        @rpath/libgcc_s.1.1.dylib (compatibility version 1.0.0, current version 1.1.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)

Of these, we can see which ones have LC_RPATH commands:

❯ fd --type=file --extension=dylib -x sh -c 'otool -L {} | rg -q rpath && echo {}' | while read dylib; do echo "===== $dylib ====="; otool -l $dylib | rg -A2 LC_RPATH || echo none; echo; done
===== ./libgcc_s.1.dylib =====
none

===== ./libobjc-gnu.4.dylib =====
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)

===== ./libgfortran.5.dylib =====
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)

===== ./libstdc++.6.dylib =====
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)

So, all libraries with an @rpath reference has the correct LC_RPATH command, except for libgcc_s.1.

I suppose you could argue that libgcc_s.1 is somehow special, but I don't see why it would be. In particular, I think the argument that

the lack of rpath (a historical feature) is not a problem since it is only supposed to be linked by gcc itself, which will add the correct path in the executable

also applies to libstdc++, etc., and yet they all contain the LC_RPATH command anyway. What makes libgcc_s.1 special?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Fix GCC and Julia so that they don't fail brew linkage --test --strict.
  • Create an allowlist for GCC and Julia that allows us to ignore brew linkage --test --strict failures.
  • Get rid of the missing RPATH check in brew linkage.

Fine with any of these with a mild preference for 1st > 3rd > 2nd.

  • Add a new flag to brew linkage that enables making indirect linkage an error in test-bot.

Would rather not do this.

@MikeMcQuaid MikeMcQuaid deleted the stricter_linkage_test branch May 18, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants