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

Tasks #2816

Merged
merged 22 commits into from
May 20, 2021
Merged

Tasks #2816

merged 22 commits into from
May 20, 2021

Conversation

fortmarek
Copy link
Member

@fortmarek fortmarek commented Apr 20, 2021

Short description 📝

This PR brings a support for Tasks.

To create a task you need to create Tasks.swift file in the root directory with a content similar to this:

import ProjectDescription
import Foundation

let tasks: Tasks = [
    .task(
        "create-file",
        options: [
            .optional("fileName")
        ]
    ) { options in
        let fileName = options["fileName"] ?? "file"
        try "File created with a task".write(
            to: URL(fileURLWithPath: "\(fileName).txt"),
            atomically: true,
            encoding: .utf8
        )
    }
]

To try the task above you can go ahead and play with it in app_with_tasks fixture.

To comment a bit more on the Tasks manifest: users have to declare a list of [Task] where each Task has a name, options and the code itself. options work in a similar way as Template.Attribute for tuist scaffold. They can be either required or optional where for required we can let the ArgumentParser check whether it exists and fail if it does not. The options are then passed to the closure as [String: String]. There you can do basically anything you want.

Current drawbacks:

  • required options still have to be unwrapped from [String: String]. I wonder if it makes sense to only allow optional options but on the other hand checking of required from ArgumentParser means that we can handle potential errors better.

I know @pepibumur that you wanted to be Tasks in a separate framework from ProjectDescription. I am not sure if I agree, though, since then e.g. dumpIfNeeded would have to be declared twice. I'd keep it as-is and move it to a different framework if it makes more sense later. But right now Tasks are still very much declarative as the other manifests.

Next steps:
After this PR gets eventually merged there are two things we should do:

  • offer access to things we know about the graph -> sources, lists of targets, etc.
  • offer some additional convenient functions for e.g. running shell processes. This might be better to add as a plugin, though, as it does not have to be in the main repo and I could imagine that other people would want to create their own set of helpers. It should be noted that such a plugin can be created already since in Tasks.swift users have access to ProjectDescriptionHelpers.

Also note that this is a draft. Once we decide that the current API is the one we want to implement, I'll tie up the loose ends like missing tests or documentation.

Checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@fortmarek fortmarek marked this pull request as draft April 20, 2021 07:01
Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

This is looking pretty nice! I'm excited to try this on some projects to automate certain setup tasks. Think it might be cool and useful to add direct support for running tuist commands. Being able to do something like: Tuist.generate(..) to automate tuist tasks would be nice as a default

public struct Task: Codable {
public let name: String
public let options: [Option]
public let task: ([String: String]) throws -> Void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we give the dictionary an alias?

public let options: [Option]
public let task: ([String: String]) throws -> Void

public enum Option: Codable, Equatable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on making this just be a struct that is optional?

Comment on lines 93 to 95
// Decoding loses information about the task's function
// This is fine as we should never invoke `task` directly but using `swiftc` command instead
task = { _ in }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Helpful comment!

import Foundation

public struct Tasks: Codable, ExpressibleByArrayLiteral {
public let tasks: [String: Task]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on just a set or array here

Sources/TuistKit/Commands/TaskCommand.swift Outdated Show resolved Hide resolved
Sources/TuistKit/Commands/TuistCommand.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Hey @fortmarek this is exciting. I left some comments regarding the approach that we are taking that I think are important to discuss to ensure we deliver a great developer experience. Let me know what you think about them :)

@@ -0,0 +1,103 @@
import Foundation

public struct Task: Codable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd create a new framework other than ProjectDescription to contain all the automation-related models. I propose naming it ProjectAutomation to be consistent with ProjectDescription and capture what the framework contains. For now it's just a model, but we'll want to include abstractions for shelling-out and doing IO operations.

}
}
@discardableResult
func dumpIfNeeded<E: Encodable>(_ entity: E) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we create the ProjectAutomation framework as proposed in one my comments, we'll need this Dump.swift file also there.

@@ -0,0 +1,36 @@
import Foundation

public struct Tasks: Codable, ExpressibleByArrayLiteral {
Copy link
Contributor

Choose a reason for hiding this comment

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

After sleeping over this, I propose something different. Let me expose the rationale first.
I'd discourage the usage of ProjectDescriptionHelpers from the Task swift files to minimize the time it takes to load the manifest. As a user, I expect the CLI to be fast and having to compile the helpers to load the tasks will lead to a bad user experience. That leads to keeping all the tasks in a single file. We know from Fastlane that something like that leads to huge and complex automation files. The solution?
I think we can have Tuist/Tasks directory where each task is represented by a swift file:

Tuist/
  Tasks/
    Release.swift
    BuildDebug.swift
    ExportApp.swift

The name of the command would be automatically inferred from the name of the task. Note that I'm exposing them directly at the root of the CLI. This is how Rails does it and developers like it:

tuist release
tuist build-debug
tuist export-app

Listing those tasks and exposing them as part of the CLI is fast. However, parsing the content of those files to register the arguments and the description is not instance because we have to resort to the compiler. We can leverage caching, but with a cold cache it'll take time, which it's not an ideal developer experience. Here are some alternatives:

  • We can parse with swift-syntax instead. Since we are parsing and not compiling, the performance should be much better.
  • Come up with a convention of comments that developers can use to annotate the task. For example:
// BuildDebug.swift
// Description: Build the app for debug
// Argument: ....

let task = Task(....

Copy link
Member Author

@fortmarek fortmarek Apr 22, 2021

Choose a reason for hiding this comment

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

I really like the structure in multiple files 👏 I agree that the current proposal would lead to a large file as Fastfiles often do.

I'd discourage the usage of ProjectDescriptionHelpers from the Task swift files to minimize the time it takes to load the manifest

Yes, compiling ProjectDescriptionHelpers will result in some performance hit, however, I believe we should allow users to share code between their templates, so something like that will be necessary (this would not have been necessary with a single file-structure but it is with multiple files). We can introduce a new framework ProjectAutomationHelpers to ensure that only the code that is needed will be compiled (in ProjectDescriptionHelpers there will be likely code not tied to tasks).

Note that I'm exposing them directly at the root of the CLI. This is how Rails does it and developers like it:

This should be possible to do but it means that we should make sure the loading is instant because loading all the dynamic arguments will have to be done every time any tuist command is invoked - not only tuist task xxx. So, if we decide to go down this path, the loading of commands and arguments should be as close to instantaneous as possible.

However, parsing the content of those files to register the arguments and the description is not instance because we have to resort to the compiler. We can leverage caching, but with a cold cache it'll take time, which it's not an ideal developer experience. Here are some alternatives:

As mentioned, we should make compile the arguments without resorting to compiler if we make the task commands part of the root tuist command

We can parse with swift-syntax instead. Since we are parsing and not compiling, the performance should be much better.

I can try to look into this and see how fast it is and how hard it will be to implement - I have never used swift-syntax, though, so might take me some time 😅 - but it should be possible and fast IMHO as swift-syntax is used for swiftformat and that tool is pretty quick to go through thousands of files

Come up with a convention of comments that developers can use to annotate the task. For example:

I think that this goes a little bit against what users are used to and why a lot of them use tuist - type-safety. If swift-syntax is not feasible or slow, rather than doing this, I'd take the performance hit (but we would have to hide behind task command, so the performance hit is not for all tuist commands)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Really like the multiple files for tasks, where the name of the file is the task name.

I do think we can get to a point where its a little confusing if we start adding more helper modules but if this would drastically speed up the time it takes to load & execute the task then its a tradeoff we can consider.

I agree with @fortmarek that comments for annotation don't fit well with the approach the project currently has, It's not type-safe and can be difficult to teach as the compiler cannot help us here, but I see how it allows us to not have to compile the file before running it. However, this concept of compiling the manifests seems to be a big selling point for Tuist and I wonder if users are okay with this trade off.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you in regards to the type-safety being one of the reasons why people choose Tuist. My inclination is towards giving swift-syntax a shot. If we manage to get almost-instant parsing without compromising the type-safety I'd go for that.
By the way, I've been playing with Rust a lot lately, and Cargo has the concept of binaries that serve the same purpose as what we are proposing here.

@apps4everyone
Copy link
Sponsor Contributor

@fortmarek is this possible to use this as a pre hook before e.g. tuist cache warm -x? Wanted to run a ./project.sh before tuist generate...

@fortmarek
Copy link
Member Author

is this possible to use this as a pre hook before e.g. tuist cache warm -x? Wanted to run a ./project.sh before tuist generate...

not yet, but I have thought about this and I think adding something like pregenerationTasks and postgenerationTasks would be a mindful addition. So something that could easily be added once this is done.

@fortmarek
Copy link
Member Author

Hey! I have updated the PR with some suggestions given and I'd like to discuss some additional things that I have not yet implemented:
Current status:

  • Tasks are created with a structure such as:
Tuist/
  Tasks/
    Release.swift
    BuildDebug.swift
    ExportApp.swift
  • Users can call the task with a kebab case version (build-debug, etc.). Currently, the tasks are hidden behind tuist task command, in which I will go into further below
  • In the file itself, an example declaration for creating a file can be the following:
import ProjectDescription
import Foundation

let task = Task(
    options: [
         .optional("fileName"),
    ]
) { options in
    let fileName = options["fileName"] ?? "file"
    try "File created with a task".write(
        to: URL(fileURLWithPath: "\(fileName).txt"),
        atomically: true,
        encoding: .utf8
    )
    print("File created!")
}

To make the calling of the tasks faster, we don't compile the task declaration when invoking it the first time to read its options, but rather use a simple regex. I have pondered using swift-syntax but have not done so since using it would tie the users of tuist to Xcode using the exact Swift version as we do (you can find more here). Regex works fine, though, albeit it is less reliable than parsing an AST.

I have not, however, per @pepibumur's suggestion surfaced these tasks from the root tuist command. This is again more of an implementation issue that I do not know how to overcome. The issue with ArgumentParser is that the parsing happens in static context of the commands - that is configuration property is static. What I'd need to do is when tuist is invoked to read the tasks and inject the new commands. But these new commands have to have their own type because their static configuration needs to differ. The only way I can think of how to do this is creating a new class via Objective-C but we are already bending ArgumentParser to something it's not supposed to do with the dynamic options. The proper way, IMHO, is to propose more dynamic elements to ArgumentParser itself. I'd like to try this at some point but is out of scope at the moment and thus I believe the tuist task workaround will have to suffice. If you, however, have and idea how to get around this, let me know - I'll try to give it some more thought as well.

The next issue I'd like to discuss is the usage of ProjectDescriptionHelpers. @pepibumur has suggested not to allow this in Tasks context because it will make the compilation of the command longer, leading to a bad user experience. While running the task on cold run will not be instantaneous, so will not be running the code and compiling the task file. I do not think compiling ProjectDescriptionHelpers will hinder performance in a substantial way - but maybe some benchmark would help make a more informative decision. That being said, ProjectDescriptionHelpers will enable users to share logic between their task files. This will make it much easier to write them. It will also allow users write plugins with tasks-specific helpers for e.g. interacting with system commands (we could build this into tuist core but this would go against trying to keep tuist core lean). Therefore, my suggestion is to allow helpers in tasks.

One other approach would be to create a new module ProjectAutomationHelpers that would be specific for tasks - this would ensure that we compile only code that is needed for the task. This goes hand-in-hand with the issue of whether Task model should be in ProjectDescription or a new module, ProjectAutomation. Both options would work in a similar fashion implementation-wise. However, I'd suggest to keep using ProjectDescription and ProjectDescriptionHelpers and that is to try to keep this feature simple. I am not yet convinced that the tasks necessitate a new module and doing so would probably confuse users where to use which module. Tuist is complicated enough for newcomers, so we should strive to not complicate it further.

I'd love to know your thoughts!

@luispadron
Copy link
Collaborator

The issue with ArgumentParser is that the parsing happens in static context of the commands - that is configuration property is static.

Ran into this when trying to wrap another program with a Swift CLI using ArgumentParser as well, it's a bummer!

I think this is fine for this feature, as it's explicit what is a standard tuist command and what is a "task".

@fortmarek I agree with a lot of your points about ProjectDescriptionHelpers. I don't know if we should assume users will have a much larger module in ProjectDescriptionHelpers vs ProjectAutomationHelpers. Users can also create plugins specific for automation if this becomes an issue. But am open to understand the use case of the other options too!

I think this PR could introduce what you have here in terms of API and we can add on with extensions to ProjectDescription that allow users to see read/write the graph, a simple file system API to performs tasks like moving company copying and to call other programs. We could include these extensions only when running tasks, to @pepibumur point.

Also happy to meet so we can discuss in more depth!

@pepicrft
Copy link
Contributor

pepicrft commented May 7, 2021

Hey @fortmarek, thanks for the thorough exploration. Let me comment on some of your points

Regex vs SwiftSyntax

SwiftSyntax requiring a dynamic link to some system's Swift components is a bummer. Let's stick to regular expressions for now. If it doesn't work as expected or there's a better version of SwiftSyntax that doesn't require dynamic links with system components we can revisit the approach. The good thing is that these things are implementation details they'll not result in breaking changes for users.

Registering tasks at the root of the CLI

This is a bummer too. Let's do the tuist tasks approach then. I was thinking about tuist run xxx but we might end up using that for running a scheme directly from the terminal. I'd rather reserve the run keyword for that.

Reusing code across tasks

While I agree there's value in sharing code across tasks, I don't think it should be in the scope of this first implementation. The introduction of ProjectDescriptionHelpers came after we saw the need and had a better understanding of what teams were doing in ther manifests. I believe the same process would apply here. We first understand what kinds of abstractions developers are doing, and then we decide if helpers make sense here, or instead, we should come up with a different abstraction.

ProjectDescription vs ProjectAutomation

Although a new framework is a new piece in the system that contributors to the project will need to get familiar with, I'm inclined towards separating things. Those frameworks provide models of different nature. The first one provides models that "describe" your projects, while the second one provides models that are intended to be "executed". The more we add to the former, the longer the compilation of the manfiests will take and the worsen the developer experience will be. As we discussed during the sync, we should provide developers with abstractions on top of Swift's IO utilities. In my opinion they should not be in ProjectDescription.


This is my proposal for how to proceed:

  • Have the tasks under tuist tasks using Regex to parse the description and the arguments.
  • Have a new framework ProjectAutomation that we link the tasks against.
  • Extend the release logic to include that framework when vendoring Tuist.
  • Document the new API
  • Define a channel for collecting feedback and keep an eye on potentially bringing helpers support to this files too.

@fortmarek
Copy link
Member Author

fortmarek commented May 7, 2021

As we discussed during the sync, we should provide developers with abstractions on top of Swift's IO utilities. In my opinion they should not be in ProjectDescription.

I agree with this - but I don't think it should be in tuist's core at all, rather in a plugin of ProjectDescriptionHelpers or something equivalent to that. This is also why I think it's better to have a Task model in ProjectDescription for now as I am not sure if it's a good idea to create a module that will include a single file - and I don't expect that we will add anything to that module anytime soon. We can of course reevaluate this later.

For the purpose of this PR, I can skip the importing of ProjectDescriptionHelpers and we can discuss it in a subsequent PR. We could bind that to defining our own "automation helpers" over Swift's IO and see then which abstraction fits best.

EDIT: Sleeping over it, I think the distinction between ProjectDescription and ProjectAutomation would be beneficial. I even think that maybe we should have done this sooner for e.g. scaffold templates that do not describe the projects and could maybe be in a different framework as well.

@fortmarek fortmarek force-pushed the tasks branch 2 times, most recently from af578e3 to 1f6571f Compare May 10, 2021 20:00
@fortmarek fortmarek marked this pull request as ready for review May 10, 2021 20:03
@fortmarek
Copy link
Member Author

I believe this is ready for proper review @luispadron @pepibumur

@dcvz
Copy link
Contributor

dcvz commented May 10, 2021

I've been waiting for something like this for a while! I'm also looking forward to the next step of being able to trigger tasks during the pre/post-generation process. Great work @fortmarek!

Regarding the "how to run a task" question. At the moment it's set on tuist task sync-translations but to me this seems passive (like this should open up Xcode to let me edit the command 😅). Instead I'd expect the cli to use an imperative command like run (which @pepibumur has already mentioned is reserved for running schemes).

If run can't be used, how about exec or run-task? In the previous example we'd have:

tuist exec sync-translations
# or..
tuist run-task sync-translations

I think exec itself is a nice descriptor of what's being done here..
run is for running a scheme and exec is for executing a task/action.

@pepicrft
Copy link
Contributor

I really like the exec idea. It reminds me to bundle exec....

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉🎉🎉🎉 This is damn awesome @fortmarek. I'd change the command to be tuist exec as @dcvz suggested and if @luispadron is onboard with the changes I'd then merge.

@fortmarek
Copy link
Member Author

Hey @dcvz, thanks for the suggestion - I do like it, too 🙌 it has been incorporated into the PR ✅

@pepicrft
Copy link
Contributor

Can't wait for this one. Running tasks from Xcode and being able to add breakpoints is a dream come true.

@fortmarek
Copy link
Member Author

Running tasks from Xcode and being able to add breakpoints is a dream come true.

Note this is not a part of this PR. But I plan to add that sooner, rather than later.

@zdnk
Copy link
Contributor

zdnk commented May 13, 2021

Will it be possible to use 3rd party SPM packages in the tasks?
Example: using Apollo tools to generate GraphQL client and queries.

@fortmarek
Copy link
Member Author

Good question @zdnk!

Example: using Apollo tools to generate GraphQL client and queries.

Isn't that possible to do that via CLI? That being said, I believe that this would make the tasks much more powerful - but is not available right now. This will need additional RFC to discuss how, if at all, we could implement this. The biggest drawback would be the longer compilation of tasks, so we should be mindful about this.

Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

Amazing stuff as always 🎉 🚀

@pepicrft pepicrft added this to the 2.0 milestone May 17, 2021
@fortmarek fortmarek force-pushed the tasks branch 3 times, most recently from 15bf1a8 to c8e9e68 Compare May 18, 2021 06:22
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.

None yet

6 participants