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

ß69: Replace UKKQueue with more modern version (VDKQueue) #944

Merged
merged 8 commits into from Jun 25, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 16, 2012

Whilst testing 64 bit QS, I got several NSInteger/int crashes, which turned out to be because of UKKQueue.

Fortunately I managed to find a modernised, GCD/64 bit fork. I have added this as a submodule and converted the code.
In the process, I had to tidy up a few areas which led me to the task viewer (and all its problems). I fixed a few small bugs there, but it's been left mostly untouched/unfixed.

The VDKQueue docs say it is 10.7+ only due to the use of @autoreleasepools - we know these build for @HenningJ on 10.6, but it would be helpful if @HenningJ could test this build before we merge - just to make sure 10.6 is safe.

pjrobertson added 8 commits Jun 14, 2012
For some reason, using GCD made the task not display properly in the task viewer (similar to how the 'update' task displays when you click the 'check for updates' button). I have no idea how to properly fix this, but have scratched my head for hours over it
@skurfer
Copy link
Member

@skurfer skurfer commented Jun 18, 2012

Apparently, I need to learn about submodules before I can test this. :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 18, 2012

OK, I tested and this seems to work from what I can tell. (I added and removed files in a “voyeured” directory and the changes were reflected immediately.)

The submodule business makes it a bit of a pain to switch between branches, though. Is it worth it for two files?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 18, 2012

The submodule business makes it a bit of a pain to switch between
branches, though. Is it worth it for two files?

Once it's merged in and all your branches have it, it shouldn't be a
problem. We can always add the 'VDKQueue' to the .gitignore.

The upside is that we pull all the latest code changes for VDKQueue
directly into QS, so we're always up to date :)

On 18 June 2012 16:24, Rob McBroom <
reply@reply.github.com

wrote:

OK, I tested and this seems to work from what I can tell. (I added and
removed files in a “voyeured” directory and the changes were reflected
immediately.)

The submodule business makes it a bit of a pain to switch between
branches, though. Is it worth it for two files?


Reply to this email directly or view it on GitHub:
#944 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 19, 2012

Once it's merged in and all your branches have it, it shouldn't be a problem.

Another thing I was going to mention is that we’d have to update the documentation on building from source. It’ll no longer be enough to clone the repo and build. People will need to set up the modules, too. If we’re going to be using more and more modules (which I’m guessing we are), then it’s no extra work, but something we need to think about.

The upside is that we pull all the latest code changes for VDKQueue directly into QS, so we're always up to date :)

Only if you update the commit that it refers to. So we have to manage one file instead of two. :-) But again, if we’re going to use modules more and more, it’s probably worth it.

So I’m guessing those two half-hearted points aren’t going to change your mind.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jun 19, 2012

I just tried it and it built fine for me.

(Well, apart from the NSURLConnectionDelegate protocol in QSCrashReporterWindowController.h, which doesn't exist on 10.6, but once you remove it, it works fine.)

Btw. I agree that git submodules are the correct way to do this, even though they are a little weird to work with.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 24, 2012

So any further problems with this? We know it works on 10.6 which is the main thing

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 25, 2012

No, I was just waiting for an answer to @HenningJ’s concerns. I didn’t get a notification since it was a comment on a commit. Merging.

Be sure to update the wiki for first-time builders.

skurfer added a commit that referenced this issue Jun 25, 2012
ß69: Replace UKKQueue with more modern version (VDKQueue)
@skurfer skurfer merged commit d375775 into quicksilver:release Jun 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants