-
Notifications
You must be signed in to change notification settings - Fork 464
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
Ruby implementation of xcproj functionality #203
The head ref may contain hidden characters: "pbxproject-write-\u{1F4A5}"
Conversation
output = `#{command} 2>&1` | ||
success = $?.exitstatus.zero? | ||
[success, output] | ||
XCODE_PATH = Pathname.new('/Applications/Xcode.app/Contents') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the Xcode path is not good. In my /Applications directory, I have
- Xcode4.app
- Xcode502.app
- Xcode511.app
- Xcode6.app
- Xcode61.app
but no Xcode.app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I always like using xcode-select -p
for this purpose
Awesome work! Be sure to take a look at this comment, though: #199 (comment) |
$stderr.reopen orig_stderr | ||
end | ||
retval | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type of output is this actually silencing? Can you paste an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of load errors for plugins, because we are initialising the IDE without actually being the IDE.
@alloy it doesn't write, though, it just touches, so it felt more appropriate to have it keep the existing API. |
What I gathered from the discussions with @0xced is that that approach is the only way that we are vulnerable to the unicode character crippling bug. i.e. we should skip the XML plist completely where possible and write to ASCII immediately. |
And regardless of immediately vs touching, there need to be safeguards against not finding the frameworks, not being able to dlopen them, symbols not being found, and runtime errors, so that we can gracefully fallback. |
Oh yes, totally - this is just an initial step. As for directly writing, I don't see how we could do that without completely relying on a |
There is no need to rely on RubyHashPropertyListWrite(hash, path)
plist = RubyHashToCFDictionary(hash)
data = objc_msgSend(plist, sel_registerName("plistDescriptionUTF8Data")) # whatever the syntax is with fiddle
write `data` (with whatever conversion is needed from NSData) to `path` |
@0xced awesome 👍 |
Right, that’s the version I meant. Thanks, @0xced. Except of course that |
So I'd say we actually put this into |
👍
I think we can get rid of it completely. The chance of one working but the other failing is slim afaik and only means more code in our codebase. |
instance, | ||
CoreFoundation.NSSelectorFromString(CoreFoundation.RubyStringToCFString('respondsToSelector:')), | ||
selector) | ||
result == CoreFoundation::TRUE ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreFoundation.CFBooleanToRubyBoolean(result)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually not a CFBoolean
, just a BOOL
This needs a rebase before it can be merged. |
@neonichu ping |
@segiddins IDK, seems to me that there is little interest in merging this, so probably should just be closed. |
@neonichu There is certainly interest in getting this into master. We've been waiting for you to rebase this branch before it's even mergable. Along with addressing the outstanding comments. |
1c58a7f
to
1fd9cbb
Compare
Well done @neonichu! Let's review this one more time, and then get this into the final 0.36 release! |
@kylef sorry, I thought I had done that already, but turns out I merged master into this instead of doing a rebase. ¯_(ツ)_/¯ |
@@ -70,6 +76,24 @@ def file_in_conflict?(path) | |||
ensure | |||
file.close | |||
end | |||
|
|||
def ruby_hash_write_xcode(hash, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neonichu Could you add some documentation to this stating which type this method expects. I.e, path
being a pathname or string etc.
- Moved implementation to `PlistHelper` - Deleted all references to `xcproj` - Added a spec for testing emoji in Xcode projects - Deliberately fall back to XML writing for some specs
Possible cases: - Xcode is not installed at all - User hasn't agreed to the license
5dacc71
to
0525ef1
Compare
Ruby implementation of xcproj functionality
awesome |
Changes Unknown when pulling 5fb3e42 on pbxproject-write-💥 into * on master*. |
see #199