textmate / swift.tmbundle

TextMate support for Swift
72 stars 30 forks source link

Treat `xcrun --sdk macosx swift` as a single command #16

Closed mkhl closed 8 years ago

mkhl commented 8 years ago

When running a script containing a correct hashbang, like

#!/usr/bin/env xcrun --sdk macosx swift

the TextMate::Executor would replace the first element of command with the parsed hashbang command.

This results in TextMate trying to execute commands like

/usr/bin/env xcrun --sdk macosx swift --sdk macosx swift

which fails because swift(1) doesn’t accept an --sdk parameter.

This change specifies the whole xcrun … swift command as a single element, which it is anyway conceptually, and which fixes this problem.

mkhl commented 8 years ago

…of course I forgot to test on a script without shebang. Such a script results in this error:

Can't find “xcrun --sdk macosx swift” on PATH.

A workaround might be disabling hashbang parsing for Swift files, otherwise I fear this’ll require changes to TM::Executor.

mkhl commented 8 years ago

I believe this has the same cause as #12

mkhl commented 8 years ago

So, the simplest thing that works: Wrap xcrun … swift in a script inside Support/bin and call that by default.

This change now works for files with and without shebangs.

sorbits commented 8 years ago

I fear this’ll require changes to TM::Executor

I assume you’re implying that TM::Executor would have to call shellsplit on the interpreter argument?

This is a change that I think would actually be good in general because many run commands will pass ENV['TM_«interpreter»'] to TM::Executor and here we may want the flexibility to include arguments or go via a shim.

Though it does cause an issue when there is a space in the path to the interpreter.

jtbandes commented 8 years ago

I assume we are talking about https://github.com/textmate/bundle-support.tmbundle/blob/master/Support/shared/lib/tm/executor.rb#L89-L91 ?

sorbits commented 8 years ago

@jtbandes Yes

mkhl commented 8 years ago

I assume you’re implying that TM::Executor would have to call shellsplit on the interpreter argument?

No, because of spaces in filenames. I was thinking about either declaring default options separately, such that they won’t duplicate options present in the shebang, or letting the first argument be an Array (like after parse_hashbang) that can be replaced wholesale.

infininight commented 8 years ago

I just tested the swift command and unless I'm mistaken the whole xcrun part is outdated now? Just running this in terminal works just fine:

#!/usr/bin/env swift

import Cocoa
print("Hello, world!")

So it seems that the macOS SDK is default now, at least with the latest (release not beta) version of Xcode. Not to short circuit talk of improving TM::Executor. :)

infininight commented 8 years ago

@mkhl I've got a commit here with a simplified Run command:

https://github.com/infininight/swift.tmbundle/commit/8545ed02b77a7fd3a9bf16b614aaceba5e9e47c5

Any problems running your code with this change?

mkhl commented 8 years ago

@infininight That works just fine, thanks.

If we merge that, should we update the shebang snippets to match? (To my understanding, you need to run xcrun --toolchain … swift if you have development versions of swift installed, but that’s probably too rare to affect the default snippet.)

infininight commented 8 years ago

Added as b3cb4372f2e63175c72eaa6e94af2eb52bb6a9db.