Move external dependencies to Dependencies group in Xcode project #186

bhargavg · 2016-03-10T15:29:17Z

Create Dependencies group in project
Move all external dependencies from Source to Dependencies group
Differentiate source packages & modules from external dependencies
Rebase and resolve conflicts
Refactor code
Add test cases

mxcl · 2016-03-11T23:25:30Z

Sources/Xcodeproj/Module+PBXProj.swift

+let rootGroupReference =                            "_____RootGroup_"
+let productsGroupReference =                        "______Products_"
+let sourcesGroupReference =                         "_______Sources_"
+let dependenciesGroupReference =                    "__Dependencies_"


There is not a requirement to have a _ prefix, so this diff can probably be less.

Agreed and minimized the diff.

bhargavg · 2016-03-11T23:28:44Z

@mxcl how are we planning to test the generated xcodeproj?

As @modocache mentioned in #174 (comment), are we going to build the targets to check whether everything is working? or do you have any thing in mind?

mxcl · 2016-03-11T23:36:01Z

I'm not sure there is anything really good we can do for testing here (within a reasonable amount of development time), keeping in mind that Xcode itself tests for a great many configurations.

So I think what we will do is run an xcodebuild test for debug and release on a few Fixtures,

bhargavg · 2016-03-12T08:48:54Z

Squashed and rebased. Please review.

aciidgh · 2016-03-12T09:07:21Z

Sources/PackageType/Package.swift

    public var dependencies: [Package] = []
    public let manifest: Manifest

-    public init(manifest: Manifest, url: String) {
+    public init(manifest: Manifest, url: String, isSourcePackage: Bool = false) {


maybe rootPackage makes more sense?

I think we're standardizing on the name “root package”.

Aside: I think there is a better name that can be found.

But we can correct this later.

mxcl · 2016-03-13T20:55:32Z

I don't want the notion of isExternalDependency to be part of PackageType this violates the responsibility boundary for this module.

Modules do not know these details about themselves. Instead this information should be passed to Xcodeproj.generate().

bhargavg · 2016-03-14T09:27:27Z

I felt this information, ie., whether it is external package or root package, would be useful not only during Xcode project generation, but also in other places in future. That's why I've added the properties isExternalDependency and isSourcePackage and set them during get() phase.

Without these properties, we have to filter through the list of packages based on path to identify whether it is root package or dependency package.

I can remove these properties and use package paths for root/dependency package identification, but I feel these properties are not violating the responsibility boundaries of these modules.

mxcl · 2016-03-14T16:40:35Z

Without these properties, we have to filter through the list of packages based on path to identify whether it is root package or dependency package.

This would also be bad, I'm not suggesting we infer the information, I'm suggesting that this information be passed differently, specifically my idea is that a Set<Module> be returned from transmute() which contains the set of packages that are external and this set be passed to other modules that need it.

mxcl · 2016-03-14T17:27:51Z

Alternatively, the Package could have a modules property. Intuitively I expect people expect this, though at this point we are only designing for ourselves.

Anyway, do which ever you like, if you have the time, otherwise I'll get around to amending this PR eventually.

bhargavg · 2016-03-17T17:53:00Z

specifically my idea is that a Set<Module> be returned from transmute() which contains the set of packages that are external and this set be passed to other modules that need it.

Sorry, I didn't get you. Could you please explain?

As of now transmute() is returning modules (root + external + test) and Products, how can we make it to return a Set<Module> and be able to identify which modules are root modules and which ones are external?

mxcl · 2016-03-17T18:12:51Z

public func transmute(packages: [Package], rootdir: String) throws -> ([Module], [Product]) {

to

public func transmute(packages: [Package], rootPackage: Package) throws -> ([Module], [Product], externalModules: Set<Module>) {

But this might not be the best way to do it, certainly it is ugly, but it will suffice for now until I get round to refactoring it.

bhargavg · 2016-03-18T20:40:20Z

@mxcl Made the changes as per your previous comment. Please review.

mxcl · 2016-03-18T21:01:09Z

Sources/Transmute/transmute().swift

-    var set = Set<Module>()
-    var stack = packages.flatMap{ map[$0] ?? [] }
-    var modules = [Module]()
+    let depPackages = packages.filter{ $0.path != rootdir }


This makes me a little uncomfortable with this, it seems fragile. We should explicitly pass in the root package.

Agreed. Refactored get() to return source package and dependency packages separately.

mxcl · 2016-03-18T22:29:09Z

Great, better even than I suggested. Thanks.

bhargavg · 2016-03-18T22:47:08Z

Could you please run CI on this?

mxcl · 2016-03-22T00:22:50Z

@swift-ci Please test

bhargavg · 2016-03-23T18:53:40Z

@mxcl, All the tests've passed. Could you please review and merge this?

kostiakoval · 2016-03-23T19:07:41Z

@bhargavg Conflicts

bhargavg · 2016-03-23T19:22:33Z

Rebased and resolved the conflicts.

mxcl · 2016-03-23T19:59:36Z

@swift-ci Please test

bhargavg · 2016-03-24T05:31:08Z

All tests passed, Could someone please merge this.

mxcl · 2016-03-24T17:54:42Z

Sorry, I wanted to try it out on a few packages first. Looks good, code-wise and result-wise.

[Xcode] Add an easy way to enable code signing.

bhargavg force-pushed the dependencies-pbxgroup branch 2 times, most recently from 0d6c64c to e7623f6 Compare March 11, 2016 23:22

mxcl reviewed Mar 11, 2016
View reviewed changes

bhargavg force-pushed the dependencies-pbxgroup branch from 9c570f1 to d56f343 Compare March 12, 2016 08:46

bhargavg changed the title ~~[WIP] Move external dependencies to Dependencies group in Xcode project~~ Move external dependencies to Dependencies group in Xcode project Mar 12, 2016

aciidgh reviewed Mar 12, 2016
View reviewed changes

bhargavg force-pushed the dependencies-pbxgroup branch from d56f343 to 5e10280 Compare March 12, 2016 10:41

bhargavg force-pushed the dependencies-pbxgroup branch from 5e10280 to 5cd0b68 Compare March 18, 2016 20:38

mxcl reviewed Mar 18, 2016
View reviewed changes

bhargavg added 2 commits March 24, 2016 00:41

Add Dependencies in project group

ba20201

Make get() return root package and external dependency packages

9d56b9a

bhargavg force-pushed the dependencies-pbxgroup branch from e3dafed to 9d56b9a Compare March 23, 2016 19:11

mxcl merged commit 2c588fd into swiftlang:master Mar 24, 2016

aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019

Merge pull request swiftlang#186 from ddunbar/xcconfig-code-signing

1370ca7

[Xcode] Add an easy way to enable code signing.

Move external dependencies to Dependencies group in Xcode project #186

Move external dependencies to Dependencies group in Xcode project #186

Uh oh!

Conversation

bhargavg commented Mar 10, 2016

Uh oh!

mxcl Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

bhargavg Mar 11, 2016

Choose a reason for hiding this comment

Uh oh!

bhargavg commented Mar 11, 2016

Uh oh!

mxcl commented Mar 11, 2016

Uh oh!

bhargavg commented Mar 12, 2016

Uh oh!

aciidgh Mar 12, 2016

Choose a reason for hiding this comment

Uh oh!

mxcl Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

mxcl commented Mar 13, 2016

Uh oh!

bhargavg commented Mar 14, 2016

Uh oh!

mxcl commented Mar 14, 2016

Uh oh!

mxcl commented Mar 14, 2016

Uh oh!

bhargavg commented Mar 17, 2016

Uh oh!

mxcl commented Mar 17, 2016

Uh oh!

bhargavg commented Mar 18, 2016

Uh oh!

mxcl Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

bhargavg Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

mxcl commented Mar 18, 2016

Uh oh!

bhargavg commented Mar 18, 2016

Uh oh!

mxcl commented Mar 22, 2016

Uh oh!

bhargavg commented Mar 23, 2016

Uh oh!

kostiakoval commented Mar 23, 2016

Uh oh!

bhargavg commented Mar 23, 2016

Uh oh!

mxcl commented Mar 23, 2016

Uh oh!

bhargavg commented Mar 24, 2016

Uh oh!

mxcl commented Mar 24, 2016

Uh oh!

Uh oh!