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 Private dependency to Manifest #74

Merged
merged 7 commits into from
Dec 17, 2015

Conversation

kostiakoval
Copy link
Contributor

Fix SR-105. Added privateDependencies to thePackage Manifest.
The privateDependencies are fetched only when building this package manually with swift-build command.

 package = Package(
    privateDependencies: [
        .Package(url: "../PrivateLib", majorVersion: 1)
    ])

swift-build will fetch and build PrivateLib

The privateDependencies are not fetched recursively. If you specify a package as a dependency that has privateDependencies, this privateDependencies wont be fetched.

let package = Package(
    dependencies: [
        .Package(url: "../Foo", majorVersion: 1)
    ])

// Foo Manifest.swift
let  package = Package(
    name: "Foo"
    privateDependencies: [
        .Package(url: "../PrivateLib", majorVersion: 1)
    ])

swift-build will fetch and build Foo but I wont fetch PrivateLib

@ddunbar
Copy link
Contributor

ddunbar commented Dec 14, 2015

Can you rebase your changes instead of having a merge commit as part of the PR?

Can we get an update to the documentation to describe this feature? At some point we are going to need some kind of "Package.swift" reference guide which is an exhaustive list of all of the things we support.

One worry, and I'm ok with merging w/o having a clear plan here: how do we ensure that a package is correct and it builds without its private dependencies? As currently scoped it seems like this feature can only be used by test targets, and it assumes we never build test targets of dependencies.

@kostiakoval
Copy link
Contributor Author

Good point, I though a bit about that too. Maybe in future it would be better to link the privateDependencies against some private target and not the mainApp target. This way when person would try to import and use private things in the public main target it will not build.

👍 I will fix rebasing and documentation.

@kylef
Copy link

kylef commented Dec 14, 2015

In the long run, I think it might be better to break out a "test" area of Package.swift (relates to @mxcl's proposal for testing support in SwiftPM) which would provide a unified way to add both test dependencies and allow adding other attributes and options in the distant future. This will be more forward thinking and offer more extensibility in the future.

For example, the approach that we was going to use in CocoaPods [1] was to have an entire specification that defines testing:

Pod::Spec.new do |spec|
  spec.name = 'URITemplate'

  spec.test_spec do |test_spec|
    test_spec.source_files = 'URITemplateTests.swift'
    test_spec.dependency 'Quick'
  end
end

Where the "test specification" has a lot of properties similar to the root specification itself.

I can see in the future, there may be need for other attributes such as source files and source file excludes similar to that available for the root package itself in SPM.

let package = Package(
  name: "Example",
  test: TestTarget(
    dependencies: [
      ...
    ],
    exclude: ["Tests/X.swift"]
  )
)

As another example, in the future there may be the ability to explicitly depend on an Apple framework in Package.swift. This is mentioned within the documentation for Package.swift. I could see that a user may want to be able to depend on specific Apple modules just within the tests:

We will add explicit support for system dependencies in the future.

let package = Package(
  name: "Example",
  test: TestTarget(
    systemDependencies: ["CoreData"]
  )
)

@fizker
Copy link

fizker commented Dec 14, 2015

I personally don't like the name privateDependencies. I like the approach of npm, where the same feature is called devDependencies.

This leaves the private word available for packages that are actually private as opposed to public, such as proprietary code hosted as a private github repo or behind similar login walls.

@ddunbar
Copy link
Contributor

ddunbar commented Dec 14, 2015

+1 on devDependencies vs privateDependencies.

@kylef I agree with your general points about carving out a special area for tests (and imagine we will want a TestTarget like thing as part of the testing proposal). I don't quite see how that conflicts with this proposal though.

This feature only added things to the package dependencies area -- it didn't address the existing issue that Target's can't refer to which part of the package dependencies they actually depend on (which is something we will need to solve). Once we had done that, all targets would presumably be able to refer to the private dependencies, or not.

Based on the discussion here, though, it starts to sound like this should be discussed with a proposal before we start picking up an implementation... given that this feature currently is really limited to being useful with tests, I think we should lump some of that discussion into the testing proposal discussion.

Thoughts?

@kostiakoval
Copy link
Contributor Author

Agree on renaming. I've fixed that and added documentation as well.

I still see the situation when devDependency would be valuable.
Example: sourceCode generators like Natalie, debugging and diagnostic tools and etc.

The testing dependencies are a bit similar to devDependency with some extra functionality (should the tests be ran when fetching package and etc.) But I don't see why this PR would conflict with further development of "Swift Testing" proposal

@mxcl
Copy link
Contributor

mxcl commented Dec 15, 2015

I don't find the current proposal suggestive that the dependencies are only for tests.

However I also think it would be tedious to force deps for tests to be specified inside test-targets, it seems likely to me you would want a test dependency for more than one test target, maybe all of them and then having to specify the dependency 3 times is tedious. Certainly we should support per-target dependencies because sometimes you need that or at least sometimes you want to be certain that some dependency isn't getting built into your tests with the associated possible global state modifications that may occur.

So, at least for now something like devDependencies seems good to me, but I'd prefer a name that indicates more clearly that these dependencies are only for tests.

@kostiakoval
Copy link
Contributor Author

Any suggestions what should be changed/fixed and what is the next steps?

@mxcl
Copy link
Contributor

mxcl commented Dec 15, 2015

It is foreseeable that dependencies would provide binaries that we could execute as part of the build, however we really want to avoid that, to avoid the hell that is current build-systems like autoconf/cmake.

So for now a name that indicates strictly test deps is preferable.

testDependencies is a little ugly, but I'd be happy with that.

@kostiakoval
Copy link
Contributor Author

I've renamed to the testDependencies

@mxcl
Copy link
Contributor

mxcl commented Dec 15, 2015

Thanks, I'll check out and review.

@kostiakoval kostiakoval force-pushed the private-dependency branch 2 times, most recently from 49ac833 to 497f2e2 Compare December 16, 2015 20:43
do {
// build the current directory
try llbuild(srcroot: rootd, targets: targets, dependencies: dependencies, prefix: builddir, tmpdir: Path.join(builddir, "\(pkgname).o"), configuration: configuration)
try llbuild(srcroot: rootd, targets: targets, dependencies: dependencies + testDependencies, prefix: builddir, tmpdir: Path.join(builddir, "\(pkgname).o"), configuration: configuration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this is not correct, since we are not building tests we should not be adding testDependencies to the dependencies list for this build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. so instead on linking testDependencies to main root targets, there should be other "testing target" that would use these testDependencies
I'm understanding that correctly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't yet support tests (there is a proposal), so there is as yet no reason to use them: just to build them.

@kostiakoval
Copy link
Contributor Author

I've removed that. Now we jest fetch and build the testDependencies

mxcl added a commit that referenced this pull request Dec 17, 2015
Add Private dependency to Manifest
@mxcl mxcl merged commit a63bc45 into swiftlang:master Dec 17, 2015
@kostiakoval kostiakoval deleted the private-dependency branch February 3, 2016 19:02
aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019
…ancel

Ignore command failures after being cancelled
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.

5 participants