Plugins: Finalize support for tuist edit #2642

luispadron · 2021-03-10T20:55:29Z

Resolves #2455

Short description 📝

This PR finalizes the work needed to allow tuist edit to work with projects that use and import plugins.

Demo

https://share.getcloudapp.com/qGuXLxN0

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.

pepicrft · 2021-03-12T16:15:41Z

Sources/TuistKit/ProjectEditor/ProjectEditor.swift

+            let config = try configLoader.loadConfig(path: path)
+            return try pluginService.loadPlugins(using: config)
+        } catch {
+            logger.warning("Failed to load plugins, attempt to fix the Config manifest and rerun the command. Continuing...")


I think here we should differentiate two scenarios:

Plugins don't exist. In that case, we shouldn't print anything and just ignore them.

The plugins fail to load. In this case, I'd expect the command to fail with a clear message on why it failed.

@pepibumur

Plugins don't exist. In that case, we shouldn't print anything and just ignore them.

This is the behavior currently, so either the config doesn't exist, or it does. In either case loadConfig will return a default config or the actual config. From here we can get the plugins, if the are no plugins, this does not warn the user and we simply continue.

The plugins fail to load. In this case, I'd expect the command to fail with a clear message on why it failed.

Are we sure we want to fail here? My understanding is we should allow the usage of tuist edit under most contexts, even if the project may be broken (since they may be using tuist edit to fix said failure). For context, this warning would be thrown if we are unable to compile either the Config.swift or any of the Plugin.swift manifests.

Fixed this up a bit, left a more detailed comment below

Sources/TuistKit/ProjectEditor/ProjectEditor.swift

pepicrft · 2021-03-12T16:17:24Z

Sources/TuistKit/ProjectEditor/ProjectEditor.swift

+
+    /// Attempts to build the loaded plugins. If it fails, shows a warning to the user.
+    /// - Returns: The built plugin helper modules.
+    private func buildPluginModules(


I think I need some context here. Why are we building the plugins for editing the project?

Sure! So for context we need to build and link plugins that are remote or that don't exist locally.

Let's break it down further:

If its a remote plugin, we first fetch it from git. From here we checkout to a tag or branch. We don't want to allow users to edit these files since they're in a cache directory and may be wiped at any time. To stop this we instead build the remote plugin and link it so that users can import it in their project.

If its a local plugin that exists outside of the directory where tuist edit was called, then this plugin wont be loaded as part of the xcode project we create for editing plugins (since we only grab plugins that are found underneath the current edit directory). In this case we take this local plugin and build it and link it as we do with the remote one. Again, this use case is only for local plugins that are in some other directory on the machine. For example:
- root -- MyPlugin -- Plugin.swift -- MyProject -- Project.swift
If we ran tuist edit from inside MyProject then we build and link MyPlugin, if instead we ran tuist edit from within root wed have MyPlugin as an editable target in the generated edit workspace so we can instead include it as a target (which means you can modify the sources, build, etc)

If its a local plugin that exists outside of the directory where tuist edit was called, then this plugin wont be loaded as part of the xcode project we create for editing plugins (since we only grab plugins that are found underneath the current edit directory).

I wonder if it would maybe make sense to load local plugins into the project.

The use case I am thinking about is the following:

I have a project with some helpers and I want to extract them to a plugin. To do that I'd appreciate if I could simultaneously edit the plugin and the project I am writing the plugins for.

To me this is the same as for remote x local Swift packages. You are not able to edit remote packages but you are able to edit the local ones.

Also, if I understand it correctly, we want to pre-build remote plugins because their sources location is something we don't want to expose - which is not the case for local plugins, though.

Would it make sense to you @luispadron to change this and make local plugins, even if they are in a different directory, editable and not pre-build them?

Yeah absolutely! I mostly didn't implement it originally because I thought it would be more complex but I have been able to simplify the logic.

So now we load both local plugins to the tuist edit command as well as any plugins that is on the current file system into the current Plugin.xcodeproj as a target.

One difference still remains: When editing with a broken config manifest that is in the directory/subdirectory of where the tuist edit command was run we are able to still load the plugin as an editable target in the project, the same is not true for local plugins outside of this execution path, since we aren't able to determine where they are located.

pepicrft · 2021-03-12T16:19:08Z

I'm leaving a general comment in regards to the approach to error handling. Catching errors and printing warnings doesn't align well with how we've been doing error handling. For every try {} catch {} here, I'd try to answer: what do I expect as a user? If you don't get that, for example, the plugins fail to load, the execution should fail. We want users to get what they expect. If we don't give them what they expect and rather print a warning they'll be left with no clue on why their request was not completed as desired.

pepicrft

This looks great @luispadron. I have a question in regards to why we need to build the plugins for the editable project.

luispadron · 2021-03-12T17:27:52Z

I'm leaving a general comment in regards to the approach to error handling. Catching errors and printing warnings doesn't align well with how we've been doing error handling. For every try {} catch {} here, I'd try to answer: what do I expect as a user? If you don't get that, for example, the plugins fail to load, the execution should fail. We want users to get what they expect. If we don't give them what they expect and rather print a warning they'll be left with no clue on why their request was not completed as desired.

I agree with this approach! I think that the tuist edit command requires a bit more thought here though, since its supposed to allow broken projects since you may be editing the manifest in order to fix said issues.

In particular, here, I think we need to decide what to do. Should we just fail execution if the Config.swift or Plugin.swift is broken? And if so is this expected in the context of tuist edit? My opinion is that we should warn in this case, since if the Config.swift is broken we can allow users to edit it using Xcode and then when they fix it they have a warning telling them to rerun the command so that we can pull in any plugins.

However, I'm curious to see what you all think. Its a tricky problem for sure

luispadron · 2021-03-13T03:13:29Z

I've updated the error handling to be more inline with what has been done before (i.e. exit with error when issues are caught).

The flow:

If we aren't able to load (compile) the Config.swift manifest
- Warn to the user but continue (this is because we should let users edit the manifests even in a broken state, so they have autocomplete, xcode support, etc)
If we aren't able to load (compile) the Plugin.swift manifest (that are found from the Config)
- Warn to the user but continue (this is because we should let users edit any local plugin manifests even in a broken state, so they have autocomplete, xcode support, etc)
If we aren't able to build remote plugins
- Exit with error. In this case the user won't be able to take any action to edit the plugins in the Xcode project (remote plugins are not loaded into the editable sources)

I believe this way we allow users to leverage Xcode to fix any broken manifests as would be expected but we inform them that since we weren't able to load the config or plugins, features like plugins will not be available (i.e. cannot import plugin module). Let me know what you think 👍🏼

fortmarek

This is looking great @luispadron!

Apart from some smaller comments I have made, I'd also reconsider the output of cloning remote plugins.

I think we should hide (when not running with --verbose) the additional git output and just print something like Downloading RemotePlugin revision ....

Otherwise, the feature works seamlessly, exciting times 🎉

fortmarek · 2021-03-13T10:36:42Z

Sources/TuistKit/ProjectEditor/ProjectEditor.swift

+        plugins: Plugins
+    ) throws -> [ProjectDescriptionHelpersModule] {
+        let loadedPluginHelpers = plugins.projectDescriptionHelpers.filter { loadedHelper in
+            !editablePluginManifestPaths.contains(where: { $0.parentDirectory == loadedHelper.path.parentDirectory })


as mentioned above, I'd change this logic to filter only remote plugins and integrate all the local plugins to the editable project directly

Had to modify the way PluginService returns the loaded plugins since we had previously lost this information (local vs. remote) during translation from description to graph model, but as you mentioned this is worth it so we can support editing all local plugins regardless of their location (as long as they can be read from Config.swift)

fortmarek · 2021-03-13T10:37:59Z

Sources/TuistKit/ProjectEditor/ProjectEditorMapper.swift

@@ -32,37 +33,37 @@ final class ProjectEditorMapper: ProjectEditorMapping {
        configPath: AbsolutePath?,
        dependenciesPath: AbsolutePath?,
        projectManifests: [AbsolutePath],
-        pluginManifests: [AbsolutePath],
+        editablePluginManifests: [(String, AbsolutePath)],


nit: I'd change this to either be a new struct or at least name the tuple values as this decreases the final readability of the code

Added EditablePluginManifest

fortmarek · 2021-03-13T10:55:11Z

Sources/TuistKit/Services/EditService.swift

-    private static var temporaryDirectory: AbsolutePath?
+    private func loadPlugins(at path: AbsolutePath) -> Plugins {
+        guard let config = try? configLoader.loadConfig(path: path) else {
+            logger.warning("Unable to load config manifest, fix and re-run to enable extra functionality.")


I wonder if we could make this a little more seamless.

How about changing the message and logic in the following way:
If we stumble upon issues with loading Config.swift, we could postpone showing this warning and instead of the final message Opening Xcode to edit the project. Press CTRL + C once you are done editing we could show the user with a warning color a message: Opening Xcode to edit the project. Plugins could not have been loaded because Config.swift could not be compiled, fix compiler errors and press ENTER. On Enter we would delete the temporary directory and regenerate the edit project.

This might be a little more complicated to implement, though, since you'd need to pass this information to ProjectEditor, so we can keep it as an improvement for next PR if you agree this would make sense.

For now, we could just make the warning message a little more explicit, eg. Unable to load Config.swift, fix any compiler errors and re-run for plugins to be loaded.

I think config manifest is not explicit enough (since manifest nomenclature is something we are familiar with internally but does not have to be immediately obvious to a new user). Plus it's not obvious what's wrong with Config.swift -> we know, though, it's because we are not able to compile it (right?), so we can add that information.

Totally agree! I think this makes sense for a follow up for sure! I wonder how difficult it would be to just listen to changes in the Config.swift or Plugin.swift and reload the project accordingly? I'll look into it as a follow up! 👍🏼

Sources/TuistKit/Services/EditService.swift

Sources/TuistLoader/Loaders/ManifestLoader.swift

features/edit.feature

features/step_definitions/shared/tuist.rb

luispadron · 2021-03-13T15:43:43Z

@fortmarek Good callout about the logging! I've updated to use verbose flag, also took the time to update how we output this information, opting to use logger.notice:

luispadron · 2021-03-14T14:35:37Z

.github/workflows/tuist.yml

@@ -79,6 +79,12 @@ jobs:
        xcode: ['12.3']
        feature:


Sorted and added edit

pepicrft · 2021-03-15T09:53:52Z

This looks great @luispadron 👏. I'll go ahead and merge the PR.

* main: Rebuild frameworks Remove unnecessary step Add Luis to the core team Fix Xcode version Fix syntax issue Bump gatsby-plugin-mdx from 1.10.0 to 2.0.1 in /next (tuist#2652) Plugins: Finalize support for tuist edit (tuist#2642) Drop support for Xcode 11.x (tuist#2651) docs: add tiarnann as a contributor (tuist#2657) Install script bug fix: Adding bin folder to usr/local/ when it is missing (tuist#2655) # Conflicts: # CHANGELOG.md

luispadron added the domain:plugin label Mar 10, 2021

luispadron self-assigned this Mar 10, 2021

luispadron force-pushed the plugins/project-n-plugins-edit branch 4 times, most recently from ef70b0c to c9a896e Compare March 12, 2021 14:28

luispadron marked this pull request as ready for review March 12, 2021 14:28

luispadron requested review from fortmarek and pepicrft March 12, 2021 15:22

pepicrft reviewed Mar 12, 2021

View reviewed changes

Sources/TuistKit/ProjectEditor/ProjectEditor.swift Outdated Show resolved Hide resolved

pepicrft reviewed Mar 12, 2021

View reviewed changes

pepicrft approved these changes Mar 12, 2021

View reviewed changes

luispadron force-pushed the plugins/project-n-plugins-edit branch 3 times, most recently from b79601b to f446eee Compare March 13, 2021 03:05

fortmarek reviewed Mar 13, 2021

View reviewed changes

luispadron force-pushed the plugins/project-n-plugins-edit branch 3 times, most recently from bfbb530 to 4442314 Compare March 13, 2021 21:37

Plugins: Finalize support for tuist edit

5eca11b

luispadron force-pushed the plugins/project-n-plugins-edit branch from 4442314 to 5eca11b Compare March 14, 2021 14:34

luispadron commented Mar 14, 2021

View reviewed changes

.github/workflows/tuist.yml

@@ -79,6 +79,12 @@ jobs:

xcode: ['12.3']

feature:

Copy link

Collaborator Author

luispadron Mar 14, 2021

Choose a reason for hiding this comment

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

Sorted and added edit

pepicrft merged commit d46543d into main Mar 15, 2021

pepicrft deleted the plugins/project-n-plugins-edit branch March 15, 2021 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugins: Finalize support for tuist edit #2642

Plugins: Finalize support for tuist edit #2642

luispadron commented Mar 10, 2021 •

edited

Loading

pepicrft Mar 12, 2021

luispadron Mar 12, 2021

luispadron Mar 13, 2021

pepicrft Mar 12, 2021

luispadron Mar 12, 2021

fortmarek Mar 13, 2021

luispadron Mar 13, 2021

pepicrft commented Mar 12, 2021

pepicrft left a comment

luispadron commented Mar 12, 2021

luispadron commented Mar 13, 2021 •

edited

Loading

fortmarek left a comment

fortmarek Mar 13, 2021

luispadron Mar 13, 2021

fortmarek Mar 13, 2021

luispadron Mar 13, 2021

fortmarek Mar 13, 2021

luispadron Mar 13, 2021

luispadron commented Mar 13, 2021

luispadron Mar 14, 2021

pepicrft commented Mar 15, 2021

Plugins: Finalize support for tuist edit #2642

Plugins: Finalize support for tuist edit #2642

Conversation

luispadron commented Mar 10, 2021 • edited Loading

Short description 📝

Demo

Checklist ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft commented Mar 12, 2021

pepicrft left a comment

Choose a reason for hiding this comment

luispadron commented Mar 12, 2021

luispadron commented Mar 13, 2021 • edited Loading

fortmarek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luispadron commented Mar 13, 2021

Choose a reason for hiding this comment

pepicrft commented Mar 15, 2021

luispadron commented Mar 10, 2021 •

edited

Loading

luispadron commented Mar 13, 2021 •

edited

Loading