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

Proposal: Revised plugin system #139

Open
orta opened this Issue Dec 15, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@orta
Copy link
Member

orta commented Dec 15, 2018

So, the current idea of plugins is great until we got it working out in prod for a while.

Roughly how it is now:

  • We detect if there are special strings in the Dangerfile before evaluation
  • If there are, we use Marathon to dynamically create a SwiftPM project, and compile those dependencies
  • The evaluation of the Dangerfile.swift then has the build products added to its runtime path, and things hook up

This is fine, but it's actually not. It takes forever to clone and build all of those dependencies and causes #97

So, instead I have two proposals:

  • Because Swift's tooling set is so much smaller, we could take SwiftLint and SwiftFormat and move them into Danger for Swift

    Roughly deprecate the need for using plugins anyway, if everyone is gonna use the same ones anyway. It's a tighter eco-system, so maybe Danger can be more focused? I'm open to change.

  • Move then idea of plugins from being owned by Danger to being owned by SwiftPM

    This is aiming to save build time, by not re-cloning, and re-building danger etc. We'd need to figure out how to make local library dependencies available to the runtime Dangerfile, but I don't think that's too tricky in my head. I've just not put enough thought into how that can actually work.

@yhkaplan

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Dec 16, 2018

Personally, I use minor plugins that I create myself sometimes, so having some kind of plugin system is preferable. One aspect of this is that many CI services are smart about caching bundled gems, npm packages, etc so installing them is very fast. Unfortunately, SPM is not big enough for CI services to optimize for yet, and probably won’t be soon.

@orta

This comment has been minimized.

Copy link
Member

orta commented Dec 16, 2018

Yeah, good point on the build caching ( I just added it to this repo )

Looks like my idea of moving your dependencies to be owned by the project won't work, because SwiftPM uses urls for resolving dependencies, it can't know that github.com/danger/swift actually represents the current project from the looks of things, will have to investigate.

@f-meloni

This comment has been minimized.

Copy link
Member

f-meloni commented Dec 16, 2018

I personally think that put all in danger has two downsides:

  • Makes the process of adding a new plugin more complicate (and impossible if i want to create a plugin specific to my company that i don't want to make opensource)
  • Limits in general the total number of generable plugins or forces everyone to have all the plugins even if they don't need them.

Today there is not a big number of plugins, I agree, but some potential plugins could be

  • XCode build/test summary
  • Coverage report
  • Lint of the xibs/nibs
  • Link jira ticket

Then i suppose that would be more future proof allow people to integrate the plugin they want.

@orta

This comment has been minimized.

Copy link
Member

orta commented Dec 16, 2018

I don't mean to deprecate plugins completely, just to make them normal packages though SwiftPM instead of as special magic dependencies added at runtime.

Then to also move the most common ones into Danger so that they are available by default, without needing the plugin system.

I now also realized that it'd just be in danger-swift where the above would be an issue, so that still makes it feasible.

@ashfurrow

This comment has been minimized.

Copy link
Member

ashfurrow commented Dec 16, 2018

Yeah, I think Danger Swift should follow the example of Cocoa architecture: common things should be easy, and uncommon things should be possible. Orta's clarification in the above comment fits that paradigm really well, and I think would be a good step forward for Danger Swift.

@orta orta referenced this issue Dec 26, 2018

Closed

Prepare for a 1.0 release #67

14 of 14 tasks complete
@orta

This comment has been minimized.

Copy link
Member

orta commented Dec 26, 2018

OK: so, what I think could work.

  • Dependencies are declared in your Package.swift
  • You define a know library product & a target attached to it:
// swift-tools-version:4.2

import PackageDescription

let package = Package(
    name: "Eigen",
    products: [
        .library(name: "Danger", type: .dynamic, targets: ["Danger"]),
    ],
    dependencies: [
      .package(url: "https://github.com/danger/swift.git", from: "1.0.0"),
      .package(url: "https://github.com/orta/danger-plugin-swiftformat.git", from: "1.0.0")
    ],
    targets: [
        .target(name: "Danger", dependencies: ["Danger", "DangerSwiftFormat"], path: "Artsy", sources: ["Stringify.swift"]),
    ]
)
  • Running danger-swift ci will then run the swift build danger for the library Danger, evaluation for the Dangerfile can run and be linked to the .build folder.

Advantages:

  • main project's SPM handles all dependencies, rather than marathon
  • single place for built artifacts (e.g. caching, and won't have to re-clone )

Downsides:

  • Has to have a target for Danger, which has to have a source file
  • There's a global namespace for targets and likely products, so this can't be done entirely by convention
@freak4pc

This comment has been minimized.

Copy link

freak4pc commented Jan 4, 2019

I really like the idea of moving the plugins as SPM package - it seems like a smart move to me and will also allow CI providers to provide better caching to these artifacts at a later phase

@f-meloni

This comment has been minimized.

Copy link
Member

f-meloni commented Jan 4, 2019

I personally like the idea of using SPM, the file in the danger target could be the Dangerfile.swift, and we could search for Danger in Sources/Danger/Dangerfile.swift.
The thing would be that if you are shipping your framework with SPM who adds your framework downloads Danger as well, because is an explicit dependency of your target and you can not comment it as dev dependency.
But maybe add // dev to the danger target as well would make Rocket hide the target as well and maybe it will work.

@f-meloni f-meloni referenced this issue Jan 12, 2019

Open

SPM danger #174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment