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

Xcode 10 support #530

Merged
merged 3 commits into from Jun 6, 2018

Conversation

Projects
None yet
6 participants
@sharplet
Copy link
Contributor

sharplet commented Jun 4, 2018

The error fix is non-breaking, but less certain about the IUO warnings.

sharplet added some commits Jun 4, 2018

@Buju77

This comment has been minimized.

Copy link

Buju77 commented Jun 5, 2018

I'm currently also having a endless recursion crash in stringify() method:

screen shot 2018-06-05 at 13 35 37

my invoking code looks like this:

var user: RTUser!

...

expect(self.user).toNot(beNil(), description: "register should never fail!")

any idea how i could fix this?

@Dschee

This comment has been minimized.

Copy link
Contributor

Dschee commented Jun 5, 2018

@Buju77 I just came across the same issue and reported it on Quick here. Looks like it's part of Nimble. Let me have a look and try to fix this real quick ...

@Dschee

This comment has been minimized.

Copy link
Contributor

Dschee commented Jun 5, 2018

I fixed this particular issue, though now I'm running into a different one. Looks like Xcode 10 support needs some more work, let me continue fixing stuff ...

@Dschee

This comment has been minimized.

Copy link
Contributor

Dschee commented Jun 5, 2018

Okay, the second issue I've found was an issue with Nimbles internal tests. When building Nimble via Carthage in a project and using it for tests, everything is working now as expected, so I fixed the above issue and sent a Pull Request onto the branch of this PR here.

@sharplet Would you mind have a look and merge, please?

@Buju77 In the meantime, you can use my fork, e.g. for Carthage users it would now be:

github "Dschee/Nimble" "xcode-10"
@Buju77

This comment has been minimized.

Copy link

Buju77 commented Jun 5, 2018

@Dschee cool thx. My tests are now working again. 👍

@sharplet

This comment has been minimized.

Copy link
Contributor Author

sharplet commented Jun 5, 2018

Discussing details of @Dschee's stringify fix over here: #532 (comment).

@sunshinejr

This comment has been minimized.

Copy link
Contributor

sunshinejr commented Jun 5, 2018

With the fix of #532 it works on my end as well, good job guys 👍

@sharplet

This comment has been minimized.

Copy link
Contributor Author

sharplet commented Jun 5, 2018

It would be nice to also have this on CI, but that will probably take a little while. Shouldn't stop us from merging this I would hope. @Quick/core where do we stand on the definition of breaking changes? This should be a source-compatible change, in which case it seems fine to include this in a patch release. Not binary compatible though.

@ikesyo

This comment has been minimized.

Copy link
Member

ikesyo commented Jun 5, 2018

I'm fine for releasing this in a patch release. 👍

@ikesyo

This comment has been minimized.

Copy link
Member

ikesyo commented Jun 5, 2018

I've ran the test suites on this branch but it failed 😱

2018-06-05 15 58 35

@Dschee

This comment has been minimized.

Copy link
Contributor

Dschee commented Jun 6, 2018

Yeah, that's the "second issue with Nimbles internal tests" I was talking about earlier. But please note that this issue only arises when running tests on Xcode 10 beta, tests are passing in Xcode 9.4 which is what we should be worried about most right now.

It's possible that the failing test in Xcode 10 is due to bugs in the beta software. And even if this was not the case, this MR is still better merged cause it works for most of us and therefore is better than a version that doesn't build at all in Xcode 10.

@sharplet

This comment has been minimized.

Copy link
Contributor Author

sharplet commented Jun 6, 2018

Is anyone at WWDC that could take this crash to the a Swift lab?

In any case, I'm going to go ahead and merge!

@phatblat

This comment has been minimized.

Copy link
Member

phatblat commented Jun 6, 2018

🙋🏻‍♂️I’m at WWDC and spending most of my time in the labs. I’ll take this to the swift lab today

@sharplet sharplet merged commit f875d73 into master Jun 6, 2018

3 checks passed

Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sharplet sharplet deleted the xcode-10 branch Jun 6, 2018

@sharplet

This comment has been minimized.

Copy link
Contributor Author

sharplet commented Jun 6, 2018

@ikesyo Interested in tagging 7.1.2?

@sharplet sharplet referenced this pull request Jun 6, 2018

Merged

Xcode 10 test fixes #534

@ikesyo

This comment has been minimized.

Copy link
Member

ikesyo commented Jun 6, 2018

I'm also at WWDC and I went to the Swift Open Hours lab and @jckarter answered that the crash may be a Swift compiler bug. A temporal workaround is

  • Removing __attribute__((noescape)) from NMBExceptionCapture.tryBlock
  • Add @escaping to public func withAssertionHandler(_ tempAssertionHandler: AssertionHandler, closure: () throws -> Void)'s closure argument
@ikesyo

This comment has been minimized.

Copy link
Member

ikesyo commented Jun 6, 2018

@sharplet Okay, I will backport this to 7.x-branch and release 7.1.2.

@phatblat

This comment has been minimized.

Copy link
Member

phatblat commented Jun 6, 2018

Lol I just got the same answer in the labs. @ikesyo did you file a radar I can dupe?

@ikesyo

This comment has been minimized.

Copy link
Member

ikesyo commented Jun 6, 2018

@phatblat No rader nor a ticket on bugs.swift.org yet

@phatblat

This comment has been minimized.

Copy link
Member

phatblat commented Jun 6, 2018

Radar: 40857699 "Swift runtime crash when closure passed as @NoEscape is mistakenly identified as having escaped (Nimble)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.