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

Cocoapods executable doesn't work on x86 machines #245

Closed
dtaylor1701 opened this issue Jan 27, 2022 · 19 comments · Fixed by #246 or #275
Closed

Cocoapods executable doesn't work on x86 machines #245

dtaylor1701 opened this issue Jan 27, 2022 · 19 comments · Fixed by #246 or #275

Comments

@dtaylor1701
Copy link
Contributor

Expected Behavior

Running the executable distributed via cocoopods in a build phase works as expected.
Example:
${PODS_ROOT}/BartyCrouch/bartycrouch lint -x --fail-on-warnings

Actual Behavior

An error is returned: bartycrouch: Bad CPU type in executable

Steps to Reproduce the Problem

  1. Run the executable found at Pods/BartyCrouch/bartycrouch

Specifications

  • Version: 4.9.0
  • Platform: iOS
  • IDE Version: Xcode 13.2.1
@Jeehut
Copy link
Member

Jeehut commented Jan 28, 2022

@dtaylor1701 Thank you for reporting this. Unfortunately, I no longer have an x86 machine available. I also didn't write the Makefile script, I only follow the instructions in the README's Contributing section. If you know how to adjust that script so it generates the x86 variant from an M1 Mac, feel free to adjust it. Otherwise, we might need use a CI automation here that could do the release steps. I don't know how many people are using this tool through CocoaPods (I personally never wanted to add support for it, it came from the community), if others have this problem, too, please report so I can prioritize accordingly. I only have limited time for maintaining BartyCrouch.

In the meantime, does going back to 4.8.1 work? The main changes were just to make BartyCrouch more configurable, but if it was working fine for you with the previous version, there's actually no reason to opt for 4.9.0. I also don't plan to make any new releases before next WWDC.

@DaanVanHirtum
Copy link

I had the same issue on an Intel Mac after updating to 4.9.0. Going back to 4.8.0 solved it.

@dtaylor1701
Copy link
Contributor Author

I've put up an edit to the Makefile which should solve the issue. #246

@Jeehut
Copy link
Member

Jeehut commented Jan 28, 2022

Thank you @dtaylor1701, I'll have a look this weekend and upload a universal binary once merged. 👍

@Jeehut
Copy link
Member

Jeehut commented Jan 30, 2022

@dtaylor1701 @DaanVanHirtum
I just uploaded a new binary to the release page, can you please try updating to 4.9.0 again and confirm if it works now?

@dtaylor1701 Thank you for the PR, unfortunately the script didn't work for me, I had to adjust some paths, see d0a60c7. But now running the script succeeds in a binary file that has double the size of the previous one, which makes sense given it's building for 2 different architectures, so looks good to me. Hope it works now.

@wassup-
Copy link

wassup- commented Jan 31, 2022

There still seems to be a problem on my x86 machine after updating to 4.9.0

dyld: Library not loaded: @rpath/lib_InternalSwiftSyntaxParser.dylib
  Referenced from: /Users/tomk/Projects/Git/xxx/Pods/BartyCrouch/bartycrouch
  Reason: image not found
/Users/tomk/Library/Developer/Xcode/DerivedData/xxx-cpuniddoybnjrvdhuosiuotrlwka/Build/Intermediates.noindex/xxx.build/Staging-Debug-iphonesimulator/xxx.build/Script-5E6493E718D2A3BBDA13ADC3.sh: line 25: 21463 Abort trap: 6           "$BARTYCROUCH" update -x
Command PhaseScriptExecution failed with a nonzero exit code

@Jeehut
Copy link
Member

Jeehut commented Jan 31, 2022

@wassup- Ahh, how I hate that error. I'm also getting it during development which prevents me from using breakpoints while coding on this project. 😞 It used to work ...

It's some kind of path problem, I think, but don't fully understand these things so I couldn't figure out a solution yet. If anyone has any tips on that, please help. The solution I'm currently aiming towards is a rewrite of BartyCrouch without any dependencies, such as swift-syntax or genstrings/extractLocStrings. I thought building on top of Apple tools isn't a bad idea (and it wasn't to get started faster) but nowadays I'd like to move away even from that. 😅

@dtaylor1701
Copy link
Contributor Author

Unfortunately it's not working for the same reason @wassup- pointed out. To meet it looks like SwiftSyntaxParser isn't being bundled with the rest of the executable.

@Jeehut
Copy link
Member

Jeehut commented Jan 31, 2022

@dtaylor1701 Do you know a way to bundle SwiftSyntaxParser with the executable? If yes, feel free to post a PR. 👍

@djbe
Copy link
Contributor

djbe commented Dec 5, 2022

Seeing as versions 4.12 & 4.13 try to address the linker issue, could you also release them on CocoaPods? Latest available there is 4.11. Hopefully we can start using newer versions (been stuck on 4.8 since this issue appeared).

@Jeehut
Copy link
Member

Jeehut commented Dec 5, 2022

@djbe IIRC there was an issue with the CocoaPods script not working with the new linker logic. But the linker logic isn't working anyways, so I will make a new release soon where I can try again and report the details here if I run into any issues again.

Having that said, I never planned to add CocoaPods support in the first place and it's more of a community thing. I'm happy to do a few steps more during release, but if things don't work, I neither have the time nor the knowledge to fix things myself for CocoaPods users. So, feel free to run the make portable_zip command and see if it works and the contents of .build/release/portable_bartycrouch.zip work for you. I'd happily merge any PR fixing things. Just note that I'm making the releases on an M1 machine.

@djbe
Copy link
Contributor

djbe commented Dec 5, 2022

No worries, I'll see if those work, and post my findings here

Update: can confirm that the current .13 release is broken, still complains about dyld[84691]: Library not loaded: '@rpath/lib_InternalSwiftSyntaxParser.dylib'. I noticed that during compilation, it complained about that dylib as well though:

ld: warning: /Users/…/BartyCrouch/.build/apple/Products/Release/lib_InternalSwiftSyntaxParser.dylib, ignoring unexpected dylib file

@Jeehut
Copy link
Member

Jeehut commented Dec 14, 2022

Hey everyone, I just made a new release 4.14.0 and also on CocoaPods (I had to specify the platform & Swift versions explicitly for the pod trunk push to work again). Note that I did some changes to how I link the .dylib for Homebrew users in the Makefile. I'm not sure how things should be done for CocoaPods users, but just as my best guess I've included a copy of the .dylib to the portable ZIP file. If things magically works somehow, I'd be surprised. If anyone using this via CocoaPods can provide more details and maybe even a fix on the issue if it's still there, I'd be happy to review the PR.

Please also report if the newly released build 4.14.0 still has issues running on x86. It should include a universal build.

@Hilehele
Copy link

Confirmed that the lib_InternalSwiftSyntaxParser issue is still present on 4.14.1, after additional deployment_target updates for iOS support (see: #273), on macOS 12.6 x86

Idea: Remove the internal static library dependency / linking logic by migrating from SwiftSyntaxParser => SwiftParser (see: 4ac0804a1, 318de5cf28a)

Rationale:

Caveat: Not sure about migration complexity, or any side-effects that it will cause for other installation methods (e.g. SPM, Homebrew)

Similar change in SwiftLint: realm/SwiftLint#4216

@djbe
Copy link
Contributor

djbe commented Dec 30, 2022

Yeah can confirm that it's not working yet, just tried it.

Had a look through the SwiftLint PR, the code changes seem very minimal (if you ignore some other unrelated changes in that PR). Sourcery also switched to it a few months ago (PR).

Regarding installation methods, for SPM there should be no change, as long as your package.swift works. Homebrew should just create a release build via SPM. Or via your makefile 🤷 For SwiftGen, our brewfile invokes our rake build, which in turn just calls swift build --disable-sandbox -c release. Non brew builds trigger a universal build with --arch arm64 --arch x86_64.

@Jeehut
Copy link
Member

Jeehut commented Dec 31, 2022

@djbe I just followed this NSHipster article until things worked: https://nshipster.com/homebrew/

I'm not an expert on Makefile at all, so if you want to introduce a different form of shipping this tool via Homebrew, feel free to suggest one. As long as I can understand it (I have some experience with Rails, so I should be able to read Ruby code) and it's as easy as it's today for me to make a release (see end of README), I'm willing to merge any changes to the build pipeline if it helps some users in the community.

Having that said, the Makefile is already using SwiftPM, the exact call for CocoaPods is (the one for Brew is here):

	swift build \
		-c release \
		--arch arm64 --arch x86_64 \
		--disable-sandbox \
		--scratch-path "$(BUILDDIR)"

How exactly is the latest CocoaPods version failing? Is it an issue with the SwiftSyntax dylib? Last thing I tried was to ship the dylib alongside the ZIP file by adding this copy line. But there may be a better way to do it. For Homebrew, copying the dylib to opt/homebrew/lib and then adjusting the lookup fixed that issue. Maybe there's something similar we can do for CocoaPods. Or ist it an entirely different issue?

@Hilehele
Copy link

Hilehele commented Jan 1, 2023

Thanks for the info @Jeehut, that definitely helps!

How exactly is the latest CocoaPods version failing? Is it an issue with the SwiftSyntax dylib?

Yes, it appears that the Pod will be unable to find the embedded dylib, which by default is located in Pods/BartyCrouch/lib_InternalSwiftSyntaxParser.dylib

Here's a more thorough error describing the issue via Xcode, upon building an app target which depends on BartyCrouch

dyld[34575]: Library not loaded: '@rpath/lib_InternalSwiftSyntaxParser.dylib'

  Referenced from: 'Path/To/Pods/BartyCrouch/bartycrouch'

  Reason: tried: '/usr/lib/swift/lib_InternalSwiftSyntaxParser.dylib' (no such file), 'Path/To/Pods/BartyCrouch/../lib/lib_InternalSwiftSyntaxParser.dylib' (no such file), '/usr/lib/swift/lib_InternalSwiftSyntaxParser.dylib' (no such file)

Potential Workaround: Update the location of the dylib to one of the search paths used by BartyCrouch (i.e. Path/To/Pods/lib/lib_InternalSwiftSyntaxParser.dylib) when installing the library via pod install

  • Rationale: Does not require changes to the Makefile, preventing possible regression with other installation methods
  • Example (using a post_install script in the Podfile, verified that the library load error is no longer observed)
post_install do |installer|
  installer.pod_targets.each do |pod_target|
    if pod_target.name == "BartyCrouch" then
      library_source_directory = pod_target.pod_target_srcroot.sub('${PODS_ROOT}', "#{installer.sandbox.root}")
      library_dest_directory = "#{library_source_directory}/../lib"

      syntax_parser_library = "lib_InternalSwiftSyntaxParser.dylib"
      system("mkdir -p #{library_dest_directory}; mv #{library_source_directory}/#{syntax_parser_library} #{library_dest_directory}/#{syntax_parser_library}")
    end
  end
end

Additional Notes:

  • Similar changes can potentially be done more generally at the Podspec-level (e.g. via script_phase)
  • Ideally we'd remove the entire dylib dependency since it's been deprecated, but more testing might be required in that case

@djbe
Copy link
Contributor

djbe commented Jan 1, 2023

As @Hilehele mentioned, it's looking for the dylib one level too high: in the Pods folder instead of in the Pods/BartyCrouch folder. A tweak of the rpath should fix this, the current rpaths are:

  • /usr/lib/swift
  • @executable_path/../lib

We should add @executable_path to that list. Tried it locally with this line, and Bartycrouch now works:

install_name_tool -add_rpath @executable_path/. Pods/BartyCrouch/bartycrouch

Until BartyCrouch switches to SwiftParser, we'll need to adapt the Makefile, particularly the portable_zip section (as it's used for the Cocoapods release). I'd recommend creating a temporary folder, copying all the artifacts in there, tweaking the rpath, and then zipping the whole thing.

portable_zip: bartycrouch_universal
	rm -f "$(BUILDDIR)/Apple/Products/Release/portable_bartycrouch.zip"
	
	$(eval TMP := $(shell mktemp -d))
	cp "$(BUILDDIR)/Apple/Products/Release/bartycrouch" "$(TMP)/bartycrouch"
	@install_name_tool -add_rpath "@executable_path/." "$(TMP)/bartycrouch"
	
	zip -j "$(BUILDDIR)/Apple/Products/Release/portable_bartycrouch.zip" \
		"$(TMP)/bartycrouch" \
		"$(REPODIR)/LICENSE" \
		".build/artifacts/swift-syntax/_InternalSwiftSyntaxParser.xcframework/macos-arm64_x86_64/lib_InternalSwiftSyntaxParser.dylib"
	echo "Portable ZIP created at: $(BUILDDIR)/Apple/Products/Release/portable_bartycrouch.zip"
	rm -rf $(TMP)

Note: I'm a noob at makefiles. If there's a better way to temporarily create a folder and tweak the binary, go ahead.
Note @Hilehele: you don't need to create folders and move files, just use the install_name_tool command above to fix your executable until is permanently fixed.

@Jeehut
Copy link
Member

Jeehut commented Jan 3, 2023

@djbe Thank you very much for the fix!
I merged it and made a new release (4.14.2), please report if there's still an issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants