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

[Add] MovieSwiftUI #360

Merged
merged 7 commits into from
Mar 30, 2020
Merged

[Add] MovieSwiftUI #360

merged 7 commits into from
Mar 30, 2020

Conversation

Dimillian
Copy link
Contributor

@Dimillian Dimillian commented Jul 20, 2019

Pull Request Description

Add MovieSwiftUI

Acceptance Criteria

To be accepted into the Swift source compatibility test suite, a project must:

  • be an Xcode or swift package manager project
  • support building on either Linux or macOS
  • target Linux, macOS, or iOS/tvOS/watchOS device
  • be contained in a publicly accessible git repository
  • maintain a project branch that builds against Swift 4.0 and passes any unit tests
  • have maintainers who will commit to resolve issues in a timely manner
  • be compatible with the latest GM/Beta versions of Xcode and swiftpm
  • add value not already included in the suite
  • be licensed with one of the following permissive licenses:
    • BSD
    • MIT
    • Apache License, version 2.0
    • Eclipse Public License
    • Mozilla Public License (MPL) 1.1
    • MPL 2.0
    • CDDL
  • pass ./project_precommit_check script run

@clackary
Copy link
Contributor

@swift-ci test

@clackary
Copy link
Contributor

Build log can be found here.

Looks like it's running into an issue with the sandbox. Looking into it to see if I can get some more details.

@Dimillian
Copy link
Contributor Author

Should I do anything within the project?

@clackary
Copy link
Contributor

I don't think a project-side change is necessary quite yet. It passes project_precommit_check for me locally, so I'm looking into the interaction with the sandbox.

@Dimillian
Copy link
Contributor Author

I've updated to a new commit, to ensure I match my correct internal SPM dependency

@clackary
Copy link
Contributor

I'll start another round of testing, but would you mind moving the MovieSwiftUI entry up to roughly line 1417? We try to keep these projects sorted by path.

@clackary
Copy link
Contributor

@swift-ci test

@Dimillian
Copy link
Contributor Author

Done :)

@clackary
Copy link
Contributor

@swift-ci test

@clackary
Copy link
Contributor

Quick update here. I filed a bug report for the sandbox issue: https://bugs.swift.org/browse/SR-11210

It's possible the sandbox issue is covering up something else. I'm going to see if I can test it locally without the sandbox to see if we hit anything. If things look good there, we may want to just add an xfail so we can get it checked in while working out the sandbox problem.

@clackary
Copy link
Contributor

@swift-ci test

@clackary
Copy link
Contributor

@swift-ci test

@varungandhi-apple
Copy link
Contributor

@swift-ci please test

@Dimillian
Copy link
Contributor Author

Dimillian commented Feb 7, 2020

Anything I can do? I've also updated the project and its swift package dependency so it can compile without warnings on Swift 5.2. Might I update the commit ref?

@varungandhi-apple
Copy link
Contributor

@Dimillian thanks for hanging in. That would certainly be helpful. If you look at the last failure here:

Resolve Package Graph

Fetching https://github.com/Dimillian/SwiftUIFlux.git

xcodebuild: error: Could not resolve package dependencies:
  sandbox-exec: sbpl1:38:21: unable to open system.sb
	(string-append "unable to open " path)

that seems a but cryptic and it isn't really clear to me what the problem was. Unless it rings any bells for you (maybe you've seen a similar error before?)... I'll try to debug that when I have some spare time later. We really would love to have MovieSwiftUI in the source compat suite sooner rather than later. 😄

@clackary
Copy link
Contributor

clackary commented Feb 7, 2020

@Dimillian Yes, a newer commit ref might help if you’ve updated the dependencies. I’ve also been able to get it building locally without issue as well, but I’ve run into issues when applying the sandbox rules.

I’ll try to sync up with @varungandhi-apple today to see if we can come up with a plan.

@Dimillian
Copy link
Contributor Author

Dimillian commented Feb 7, 2020

Is there a way for me to simulate the CI sandbox rules on my machine?

@clackary
Copy link
Contributor

clackary commented Feb 7, 2020

I don't think the exact rules we use are public for security concerns, but the basic idea of the sandbox rules is to limit write access to only within swift-source-compat-suite. You might be able to roll your own .sb file with local paths to simulate it, but that can get hairy.

I've opened #408 as a possible solution here. Turns out there's an xcodebuild flag that will disable the package support sandbox, and hopefully prevent the collision.

@Dimillian
Copy link
Contributor Author

I've updated to the latest commit, and updated the Swift version to 5.2

@clackary
Copy link
Contributor

clackary commented Feb 8, 2020

@swift-ci test

@varungandhi-apple
Copy link
Contributor

varungandhi-apple commented Feb 9, 2020

I think there are two kinds of compiler failures going on:

  1. The protocol conformance DispatchQueue : Scheduler is not being found. This one is getting triggered by MovieSwiftUI. This is being tracked in SR-12077.
  2. There are one or more bugs related to the name mangler. These are getting triggered by ReactiveKit, swift-nio and others. It doesn't look like there have been any changes to the mangling code recently, so maybe the problem is somewhere else. I've filed SR-12166.

I don't think we can merge this before those are fixed. 🤕

@varungandhi-apple
Copy link
Contributor

@swift-ci test

1 similar comment
@shahmishal
Copy link
Member

@swift-ci test

@shahmishal shahmishal merged commit d99deaa into swiftlang:master Mar 30, 2020
@varungandhi-apple
Copy link
Contributor

Thanks for merging this! 🎉

@Dimillian
Copy link
Contributor Author

Woooow thanks !!!! 🎉

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.

4 participants