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

Fix the bug I introduced in #1030 #trivial #1035

Merged
merged 1 commit into from
Jul 17, 2018
Merged

Conversation

Adlai-Holler
Copy link
Member

  • Major bug in the release: I was releasing the layout itself, not the layout element.
  • Improvement in the retain: Previously I bridge-casted then retained (= [[[o retain] autorelease] retain]), now I use a bridging retain (= [o retain]).

Thanks Michael for recognizing that the CI simulator loop was caused by this. I don't know why it resulted in a simulator loop but at least it's fixed.

@Adlai-Holler Adlai-Holler changed the title Fix the bug I introduced in #1030 Fix the bug I introduced in #1030 #trivial Jul 17, 2018
@ghost
Copy link

ghost commented Jul 17, 2018

🚫 CI failed with log

@Adlai-Holler
Copy link
Member Author

🤔 Hmmmm... A CALayer behavior test?? Did we change the OS version or SDK version or Xcode version or anything like that recently? Will look at this soon.

@Adlai-Holler
Copy link
Member Author

Hmm I see the same test failure locally when I run against the last good master commit 5cad23b which passed CI before landing in #1028.

The CI uses the 11.0 SDK which I don't currently have (I have 11.4). Downloading old Xcode and will try that.

Even if this doesn't fix the remaining issue, I believe the diff is worth landing.

@ghost
Copy link

ghost commented Jul 17, 2018

🚫 CI failed with log

@Adlai-Holler
Copy link
Member Author

Duplicating the exact Xcode version and simulator setup as the CI, I still see this same failure cases (CALayerTests fail, simulator loops) on the head of any diff before #1030 (all of which passed CI.)

@maicki You said that build.sh tests passes on your machine for commits before #1030? Are you sure? Does it do so consistently?

@garrettmoon You mentioned you upgraded the CI OS today. What upgrade?

In any case, I will keep looking into this tomorrow but for now I will land this, since it is certainly worth landing. I believe it fixes many but not all issues that we currently face.

@Adlai-Holler Adlai-Holler merged commit cf810ac into master Jul 17, 2018
@Adlai-Holler Adlai-Holler deleted the AHFixElementRetain branch July 17, 2018 03:44
mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Oct 2, 2018
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.

2 participants