#1148 - Add CARTHAGE_PATH env var with executable path

[auibrian]

Per @schwa's request (#1148) and @kballard's idea this adds a $CARTHAGE environment variable which is set to the path of the currently running carthage executable so that the build can:

• know it is running under carthage
• call copy-frameworks, etc with the same version of carthage if desired.

[lilyball]

The resulting $CARTHAGE variable contains the path to the directory containing carthage, not the path to carthage itself. That's not very useful.

[bhargavg]

Isn't this almost same as #1151 ?

[mdiep]

Isn't this almost same as #1151 ?

Yes it is—except this aims to make the value the path to carthage, instead of just a simple YES. Is there a reason why we should prefer this?

[auibrian]

The second point in the description is the main reason for preferring this over YES: it would allow for run script phases to use the same carthage binary that the build was being exec'd by. I can split it out into something like CARTHAGE_PATH as well. I'm just updating it now to contain the path to carthage itself.

[bhargavg]

👍 @auibrian, I like the idea of having the CARTHAGE_PATH available to build scripts.

I guess, instead of two vars, we can just have one variable CARTHAGE_PATH that will be set to path of carthage if installed, else, will not be set at all.

[auibrian]

Done. I've made it so that it will set CARTHAGE_PATH if the binary name contains carthage at all.

[lilyball]

Why are you fetching the path twice here via two different properties? Also, calling lowercaseString is unnecessary work when you can just do a case-insensitive compare, and using rangeOfString() seems incorrect because it'll also trigger on a tool named thisIsNotCarthage or somesuch. I'd recommend something like

if let executableURL = NSBundle.mainBundle().executableURL
    where executableURL.lastPathComponent?.caseInsensitiveCompare("carthage") == .OrderedSame,
    let executablePath = executableURL.path {
        args += [ "CARTHAGE_PATH=\(carthagePath)" ]
}

[auibrian]

I truly don't remember my thinking in changing to use executablePath as well as executableURL, that should be fixed.

The use of lowercaseString is directly a result of using rangeOfString() and the reason for that was an explicit desire to catch the use of any binary with carthage in the name. I was envisioning the binary names being carthage_0.14 or along those lines. Matching against a tool named thisIsNotCarthage would be an undesirable effect of course. I could conceivably use a proper regex to make sure carthage only featured at the beginning of the string but then someone might name the binary carthageIsNotThisTool.

I'm not really sure if it is preferable to only catch the canonical name carthage or to allow a bit of leeway in case users rename their binaries. If it doesn't end up only working for carthage I will update it to use localizedStandardContainsString() rather than lowerCaseString.rangeOfString().

[lilyball]

localizedStandardContainsString() would be a weird thing to use here. This is an executable name, not a localized string, and the user's current selected locale should not in any way affect whether the executable is considered to be Carthage.

[auibrian]

Fair enough, lowercaseString.containsString() it is, assuming we want to allow for the renaming of the binary at all.

[lilyball]

That's still unnecessary work. Why construct a new lowercase string when it's trivial to just do a lowercase compare? I still disagree about doing rangeOfString, because people don't typically rename executables, but if you really want to, then you can use rangeOfString("carthage", options: [.CaseInsensitiveSearch]) != nil.

[auibrian]

My thinking was around people who had multiple versions installed, maybe a beta and a release version. If it's not considered helpful I can make the check just for carthage

[lilyball]

Can you even do that? The carthage binary relies on CarthageKit.framework, which is installed in /Library/Frameworks. I don't see how you can possibly have multiple versions of Carthage installed unless you put them in different directories with a copy of CarthageKit.framework in the same directory (that's the only way to get it to load a different version of CarthageKit.framework than the one in /Library/Frameworks). And if they're in different directories then there's no reason to rename the executable.

[mdiep]

I've merged #1151 so that the CARTHAGE variable will be available when building with CarthageKit (which carthage uses).

I'd like to also make this environment variable available—but I'd like to make it part of carthage and not CarthageKit. I think we should be able to set the variable in main.swift. That will (1) allow us to get rid of the check to see if the executable name is carthage and (2) allow people to distinguish between carthage and another project that uses CarthageKit.

Would you be willing to update the PR to do that?

[auibrian]

No problem, I'll look into it.

[mdiep]

@auibrian Are you still interested in updating this PR?

[auibrian]

I am, I'll update it in the next few days. Thanks!

[auibrian]

@mdiep Alright, this is ready to go now. Sorry for the delay.

[mdiep]

Thanks again for the PR!