Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

swiftgen 0.5.0 fix #45008

Closed
wants to merge 1 commit into from
Closed

swiftgen 0.5.0 fix #45008

wants to merge 1 commit into from

Conversation

AliSoftware
Copy link
Contributor

Given this issue, we can't use bottles to install SwiftGen

The Problem

What happens is that being a Swift script, it depends on Swift dylibs that are located in Xcode.app bundle, and the binary's @rpath points to the path to Xcode.app that was used to build the binary, in order to find those dylibs back when running.

If the user's Xcode.app is not in the same location as the Xcode.app used by BrewBot to build the bottle, then the binary will have an incorrect @rpath and the Swift libraries won't be found.

What I tried

Instead of completely remove the support for the bottle, I first tried to fix the installed binary using a post_install, but to no avail, because when trying to run install_name_tool -add_rpath /path/to/Xcode/in/local/machine/Contents/… /usr/local/bin/swiftgen to fix that @rpath, it fails with a Permission Denied error. Here is the post_install code I tried for reference.

def post_install
    if build.bottle?
      # Fix the path to Xcode's Swift dylibs if it's installed via bottle and Xcode is at a different path
      swiftc = Pathname.new `xcrun -find swiftc`
      rpath = swiftc/"../../lib/swift/macosx"
      # Problem here: if the rpath already exists, this command will fail. Can't find a way to check that first
      system(%Q(install_name_tool -add_rpath "#{rpath}" "#{bin/"swiftgen"}"))
    end
  end

Result:

error: /Applications/Xcode 7.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool: 
can't open input file: /usr/local/Cellar/swiftgen/0.5.0/bin/swiftgen for writing (Permission denied)

Workaround

Even if we find a way to fix it, in the meantime I need to disable the bottle so that users can run it at least on any machine, whatever their Xcode.app path is.

cellar :any
sha256 "29391f183e1606e50008ad148b3665d240c28ab5cbd42a293d1ca99b29aa3793" => :el_capitan
sha256 "c150775bf2f6b8e77a821eae7bd89233a134eb22ce5eed88de9eaf2822dd79ee" => :yosemite
def pour_bottle?
Copy link
Member

Choose a reason for hiding this comment

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

This is too harsh, IMO. Rather than banning bottles for everyone it would seem to make more sense to check whether Xcode was installed in the default location. Honestly I'd be surprised if there are many people not running Xcode out of /Applications though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a LOT of people having Xcode in /Applications but not named Xcode.app.

Especially now as people (like me) tend to keep an Xcode6.app around for some time for legacy projects, also have Xcode7.app for everyday projects, and also have Xcode7.1beta.app for testing tvOS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any guarantee that the machine BrewBot uses to build the bottle use an Xcode named /Applications.Xcode.app all the time? In that case I can indeed only use the bottle if it's also the case for the user.

Copy link
Member

Choose a reason for hiding this comment

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

Given a not tiny number of compile scripts specifically check for /Applications/Xcode.app and presume you don't have one unless it is named exactly that, having no Xcode.app is quite risky. You can keep multiple Xcodes, but you'll hit significantly fewer issues if one is kept as the default name.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any guarantee that the machine BrewBot uses to build the bottle use an Xcode named /Applications.Xcode.app all the time?

On stable OS X versions, yes. On very beta OS X versions it may well be Xcode-beta.app but 10.9, 10.10 & 10.11 are all stable. The first runs Xcode 6.4, the latter two run 7.0.1, all three are /Applications/Xcode.app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given a not tiny number of compile scripts specifically check for /Applications/Xcode.app and presume you don't have one unless it is named exactly that, having no Xcode.app is quite risky. You can keep multiple Xcodes, but you'll hit significantly fewer issues if one is kept as the default name.

Sure, but:

  • those scripts are being bad citizens, and I prefer to be a good one
  • that's not always anybody's choice.

For example in the case of the OP of SwiftGen/SwiftGen#35 (comment) the issue is on CircleCI, not even his own machine. So I think I should still ensure it works on such contexts.

Copy link
Member

Choose a reason for hiding this comment

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

Of course. Not saying you can't or wouldn't want to work on a fix, just saying that banning everyone from using bottles is probably a bit too broad in this particular situation when we can do a check to see where Xcode is installed and pour if true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agreed, and just fixed. Thx!

@MikeMcQuaid
Copy link
Member

Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@AliSoftware AliSoftware deleted the swiftgen-0.5.0-fix1 branch October 25, 2015 18:23
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants