Add post build check to verify that the libs on macOS have the right architecture #163

autoantwort · 2021-08-18T10:56:25Z

Fixes microsoft/vcpkg#19623

Neumann-A · 2021-08-18T11:14:17Z

hmm can this be generalized to include apple and normal linux with objdump instead?

neumann@heineken:~$ objdump -f /usr/lib/libvmtools.so.0

/usr/lib/libvmtools.so.0:     file format elf64-x86-64
architecture: i386:x86-64, flags 0x00000150:
HAS_SYMS, DYNAMIC, D_PAGED
start address 0x000000000001f9e0

autoantwort · 2021-08-18T11:20:52Z

I don't know objdump very good, but it seems that it does not support universal binaries:

➜  vcpkg git:(update-fontconfig) objdump -f /Applications/Firefox.app/Contents/MacOS/libosclientcerts.dylib
➜  vcpkg git:(update-fontconfig) lipo -archs /Applications/Firefox.app/Contents/MacOS/libosclientcerts.dylib
x86_64 arm64

it also returns sometimes errors where lipo just works:

➜  vcpkg git:(update-fontconfig) objdump -f  /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/gettext_x64-osx/debug/lib/libintl.a 

/Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/gettext_x64-osx/debug/lib/libintl.a(bindtextdom.o):   file format mach-o arm64

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/objdump: error: 'bindtextdom.o': Invalid/Unsupported object file format
➜  vcpkg git:(update-fontconfig) echo $?                                                                                                          
1

autoantwort · 2021-08-18T11:23:08Z

Here an example that shows that it works:

Log

$ vcpkg install --triplet=x64-osx --host-triplet=x64-osx --binarysource=clear
... 
Starting package 2/3: gettext:x64-osx
Building package gettext[core,tools]:x64-osx...
-- Downloading https://ftp.gnu.org/pub/gnu/gettext/gettext-0.21.tar.gz;https://www.mirrorservice.org/sites/ftp.gnu.org/gnu/gettext/gettext-0.21.tar.gz -> gettext-0.21.tar.gz...
-- Cleaning sources at /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/buildtrees/gettext/src/0.21-d3cbdc875d.clean. Use --editable to skip cleaning for the packages you specify.
-- Extracting source /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/downloads/gettext-0.21.tar.gz
-- Applying patch 0002-Fix-uwp-build.patch
-- Applying patch 0003-Fix-win-unicode-paths.patch
-- Applying patch rel_path.patch
-- Applying patch android.patch
-- Using source at /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/buildtrees/gettext/src/0.21-d3cbdc875d.clean
-- Getting CMake variables for x64-osx-rel
-- Configuring x64-osx-rel
-- Building x64-osx-rel
-- Installing x64-osx-rel
-- Configuring x64-osx-dbg
-- Building x64-osx-dbg
-- Installing x64-osx-dbg
-- Performing post-build validation
The following files were built for an incorrect architecture:

    /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/gettext_x64-osx/debug/lib/libintl.a
Expected x64, but was: arm64


Found 1 error(s). Please correct the portfile:
    /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/ports/gettext/portfile.cmake
-- Performing post-build validation done
Error: Building package gettext:x64-osx failed with: POST_BUILD_CHECKS_FAILED
Please ensure you're using the latest portfiles with `./vcpkg update`, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
  Package: gettext:x64-osx
  Vcpkg version: 2021-08-12-unknownhash-debug

Additionally, attach any relevant sections from the log files above.

strega-nil-ms

This looks really good; we should also set up testing for arm64-osx I think.

src/vcpkg/postbuildlint.cpp

ras0219-msft · 2021-08-20T22:02:41Z

src/vcpkg/postbuildlint.cpp

+            return LintStatus::ERROR_DETECTED;
+        }
+#elif defined(__APPLE__)
+        std::vector<FileAndArch> binaries_with_invalid_architecture;


I think this would be better implemented as a separate function unless it can actually share some of the implementation.

Yeah the function name was very matching and it is doing the, but now for mac. If you want I can change it, but I would prefer the current solution.

src/vcpkg/postbuildlint.cpp

autoantwort · 2021-09-24T16:07:00Z

What is the state of this PR?

src/vcpkg/postbuildlint.cpp

strega-nil-ms · 2021-10-06T19:00:11Z

It looks like this has an infinite loop; can you take a look?

autoantwort · 2021-10-08T09:38:29Z

Ready now.

include/vcpkg/base/util.h

src/vcpkg/postbuildlint.cpp

autoantwort · 2021-11-30T02:21:34Z

Any news?

…architecture

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

strega-nil-ms · 2021-12-29T19:55:23Z

I do want @ras0219-msft's review on this.

autoantwort · 2022-01-13T17:00:59Z

Ping @ras0219-msft for review :)

ras0219-msft

Approved after my comments are addressed. Thanks for the PR and happy holidays!

src/vcpkg/postbuildlint.cpp

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

src/vcpkg/postbuildlint.cpp

BillyONeal · 2022-02-10T23:21:55Z

Nicole poked Robert in person about taking another look at this one today.

ras0219-msft

I think there are still a few deficiencies, but I don't think this makes our current situation worse except perhaps cross-compiling from MacOS to Windows with MinGW, which according to my reading will now crash when lipo fails to understand the PE binaries.

I would like to see the hard crashes downgraded to warnings, but it's only a suggestion and I see no blocking issues for this PR.

src/vcpkg/postbuildlint.cpp

dg0yt · 2022-03-09T12:22:30Z

except perhaps cross-compiling from MacOS to Windows with MinGW

... or mingw in general? #23450

autoantwort mentioned this pull request Aug 18, 2021

Make macOS binary cache deterministic. Workaround for #19623 microsoft/vcpkg#19624

Closed

autoantwort force-pushed the do-macOS-arch-check branch 2 times, most recently from de502ef to 8377d64 Compare August 18, 2021 22:11

strega-nil-ms approved these changes Aug 19, 2021

View reviewed changes

ras0219-msft requested changes Aug 20, 2021

View reviewed changes

autoantwort force-pushed the do-macOS-arch-check branch 5 times, most recently from 842bd26 to d28f208 Compare August 24, 2021 00:53

autoantwort requested a review from ras0219-msft September 9, 2021 09:59

strega-nil-ms reviewed Sep 27, 2021

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort force-pushed the do-macOS-arch-check branch 4 times, most recently from c074b8e to 7c2a157 Compare October 6, 2021 11:18

autoantwort force-pushed the do-macOS-arch-check branch from 7c2a157 to 3b2504d Compare October 7, 2021 22:17

strega-nil-ms suggested changes Oct 18, 2021

View reviewed changes

autoantwort and others added 5 commits December 29, 2021 11:50

Add post build check to verify that the libs on macOS have the right …

ad10505

…architecture

Fix NotExtensionsCaseInsensitive

1d4e14a

trim lipo output

d7b954e

Apply suggestions from code review

b709ccf

Co-authored-by: nicole mazzuca <83086508+strega-nil-ms@users.noreply.github.com>

fix build

75a6f7c

fix build

23a1539

strega-nil-ms approved these changes Dec 29, 2021

View reviewed changes

remove unnecessary contains function

4caa455

strega-nil-ms force-pushed the do-macOS-arch-check branch from c3e99cd to 4caa455 Compare December 29, 2021 19:56

ras0219-msft reviewed Jan 13, 2022

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort and others added 3 commits January 16, 2022 23:11

Update src/vcpkg/postbuildlint.cpp

18410e4

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

Fix MinGW detection

5a8feef

format

d202291

autoantwort mentioned this pull request Feb 7, 2022

[vcpkg] Add universal builds for macOS microsoft/vcpkg#22978

Closed

Osyotr reviewed Feb 7, 2022

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

ras0219-msft approved these changes Feb 17, 2022

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

Apply CR. Do checks based on target triplet

1c08cdd

strega-nil-ms approved these changes Mar 7, 2022

View reviewed changes

strega-nil-ms merged commit cb47927 into microsoft:main Mar 7, 2022

autoantwort deleted the do-macOS-arch-check branch August 9, 2022 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add post build check to verify that the libs on macOS have the right architecture #163

Add post build check to verify that the libs on macOS have the right architecture #163

autoantwort commented Aug 18, 2021 •

edited

Loading

Neumann-A commented Aug 18, 2021

autoantwort commented Aug 18, 2021

autoantwort commented Aug 18, 2021

strega-nil-ms left a comment

ras0219-msft Aug 20, 2021

autoantwort Aug 20, 2021

autoantwort commented Sep 24, 2021

strega-nil-ms commented Oct 6, 2021

autoantwort commented Oct 8, 2021

autoantwort commented Nov 30, 2021

strega-nil-ms commented Dec 29, 2021

autoantwort commented Jan 13, 2022

ras0219-msft left a comment

BillyONeal commented Feb 10, 2022

ras0219-msft left a comment •

edited

Loading

dg0yt commented Mar 9, 2022 •

edited

Loading

Add post build check to verify that the libs on macOS have the right architecture #163

Add post build check to verify that the libs on macOS have the right architecture #163

Conversation

autoantwort commented Aug 18, 2021 • edited Loading

Neumann-A commented Aug 18, 2021

autoantwort commented Aug 18, 2021

autoantwort commented Aug 18, 2021

strega-nil-ms left a comment

Choose a reason for hiding this comment

ras0219-msft Aug 20, 2021

Choose a reason for hiding this comment

autoantwort Aug 20, 2021

Choose a reason for hiding this comment

autoantwort commented Sep 24, 2021

strega-nil-ms commented Oct 6, 2021

autoantwort commented Oct 8, 2021

autoantwort commented Nov 30, 2021

strega-nil-ms commented Dec 29, 2021

autoantwort commented Jan 13, 2022

ras0219-msft left a comment

Choose a reason for hiding this comment

BillyONeal commented Feb 10, 2022

ras0219-msft left a comment • edited Loading

Choose a reason for hiding this comment

dg0yt commented Mar 9, 2022 • edited Loading

autoantwort commented Aug 18, 2021 •

edited

Loading

ras0219-msft left a comment •

edited

Loading

dg0yt commented Mar 9, 2022 •

edited

Loading