Use BUILD_WORKSPACE_DIRECTORY in SwiftLintPlugin

[garricn]

Changelog

• Rewrite SwiftLintPlugin using BUILD_WORKSPACE_DIRECTORY without relying on the --config option
• Rewrite Plugins/SwiftLintPlugin/Path+Helpers.swift
• Update Plug-in Support in README.md

Benefits

• Supports parent/child configurations
• Supports nested configurations

Background

The current version of the plugin uses the --config option which is incompatible with project structures using parent/child configurations or nested configurations.

• References:
  • https://github.com/realm/SwiftLint/blob/0.50.3/Plugins/SwiftLintPlugin/SwiftLintPlugin.swift#L33
  • https://github.com/realm/SwiftLint/blob/0.50.3/Plugins/SwiftLintPlugin/SwiftLintPlugin.swift#L79

Instead of relying on the --config option, the plugin can leverage the BUILD_WORKSPACE_DIRECTORY environment variable (recently added to support Bazel) which sets SwiftLint’s working directory.

• Reference: https://github.com/realm/SwiftLint/blob/0.50.3/Source/swiftlint/Commands/SwiftLint.swift#L7

Implementation Details

The plugin determines the SwiftLint working directory by locating the topmost config file within the package/project directory. If a config file is not found therein, the package/project directory is used as the working directory.

Project Structures

The plugin throws an error when it is unable to resolve the SwiftLint working directory. For example, this will occur in Xcode projects where the target's Swift files are not located within the project directory.

To maximize compatibility with the plugin, avoid project structures that require the use of the --config option.

Unexpected Project Structures

Project structures where SwiftLint's configuration file is located outside of the package/project directory are not directly supported by the build tool plugin. This is because it isn't possible to pass arguments to build tool plugins (e.g., passing the config file path).

If your project structure doesn't work directly with the build tool plugin, please consider one of the following options:

• To use a config file located outside the package/project directory, a config file may be added to that directory specifying a parent config path to the other config file, e.g., parent_config: path/to/.swiftlint.yml.
• You can also consider the use of a Run Script Build Phase in place of the build tool plugin.

[tonyarnold]

I'm happy to see a cleaner option for configuration discovery!

Also happy to see #4757 rolled into this.

[tonyarnold]

I've just given this a run, and it's not working for Xcode targets:

working directory:
   /Users/tonyarnold/Developer/Itty Bitty Apps/Reveal/Application
tool mapping:
   swiftlint: /Users/tonyarnold/Library/Developer/Xcode/DerivedData/Reveal-ezgnpwiedoerqlgbbonoifrichyk/SourcePackages/artifacts/swiftlint/SwiftLintBinary.artifactbundle/swiftlint-0.50.3-macos/bin/swiftlint
tool search paths:
   /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
   /Applications/Xcode.app/Contents/Developer/usr/bin
   /bin
   /sbin
   /usr/bin
   /usr/sbin

failedToResolveSwiftLintWorkingDirectory

This is a project that uses an Xcode workspace, so the Xcode projects are held in subdirectories of the overall root of the workspace.

In this instance, the .swiftlint.yml is held in /Users/tonyarnold/Developer/Itty Bitty Apps/Reveal, which is the parent of the Xcode project directory.

The previous implementation of this build plugin continued walking up through the filesystem beyond the build plugin's concept of "target root", and I believe that this new implementation should do the same. It's going to be a massive inconvenience for my projects to have to move back to bash scripts due to an implementation detail if this change is merged as-is.

[garricn]

@tonyarnold Thank you for your comments and for giving it a try! The Plugin does work for Xcode targets, but only when the config file is located within "the package/project directory (and its sub-directories)" as mentioned in the PR description.

We took this approach as it ensures support for the most significant number of predictable project structures.

We can consider traversing the filesystem above the project directory as you suggest. Before we do that, however, we'd like to propose using symlinks for unpredictable project structures, such as when the config file is contained in an ancestor or sibling directory.

We've successfully tested this approach with an example project matching your setup.

To test this, a symlink that links to the config file in the parent directory should be added to each of your project directories, i.e. ../.swiftlint.yml.

If your test is successful, we will update the Project Structures section of the PR description with the following:

To provide the plugin access to a config file located outside the package/project directory, 
a symlink that links to the config file should be added to the package/project directory.

We're excited to receive the results of your test. Thank you!

[garricn]

@tonyarnold We've gone ahead and proactively updated the Plug-in Support section of the README.md including the information about the symlink solution. We've updated the PR description to match.

[tonyarnold]

I couldn't get the symlinks to work reliably, so I placed a .swiftlint.yml file in each of my project directories containing:

parent_config: ../.swiftlint.yml

I think it's messy, but it solves the problem.

[tonyarnold]

Does running the SwiftLint CLI in a directory without a configuration file throw an error? It'd be a pretty rare combination to just accept the out-of-the-box defaults, right?

[garricn]

I couldn't get the symlinks to work reliably, so I placed a .swiftlint.yml file in each of my project directories containing:

parent_config: ../.swiftlint.yml

I think it's messy, but it solves the problem.

Hey @tonyarnold, we appreciate that and understand that the parent config option can seem messy for a setup like yours. However, we believe that it's an ideal approach for the plugin since it's a supported feature of SwiftLint. Thank you for continuing to test this out and for providing feedback. We've updated the README and the PR description with information about using parent configs when needed.

[garricn]

Does running the SwiftLint CLI in a directory without a configuration file throw an error? It'd be a pretty rare combination to just accept the out-of-the-box defaults, right?

Hey @tonyarnold, we tested this locally and a config file is not required. We updated the plugin to run SwiftLint in the package/project directory if it's unable to locate a working directory containing a config file. Thank you for pointing this out!

[tonyarnold]

Awesome, this is looking great and it will work for what I use the plugin for based on my testing. Thanks for the work you've put into this!

Do you have a team at your end working on this, @garricn? Gosh, if only all OSS contributors were so lucky!

[garricn]

Awesome, this is looking great and it will work for what I use the plugin for based on my testing. Thanks for the work you've put into this!

Do you have a team at your end working on this, @garricn? Gosh, if only all OSS contributors were so lucky!

Thank you for your recognition @tonyarnold. We really appreciate it and are glad this solution will work for you.

We have not had many opportunities to contribute to open-source projects. However, our team is working on internal Swift packages and SwiftLint is crucial for maintaining quality and consistency across our Swift repositories. As our internal contributor base has grown, we identified that it is essential to have violations appear in Xcode. Once we discovered that parent/child and nested configurations (which we rely on) were not being respected, we immediately committed ourselves to improve the plugin.

I assume you're asking about the team because I keep using "we". I'm working on this with @chrisfuller.

TL;DR … this contribution is of particular importance to our team and the projects we work on and we hope it gets merged soon. 🤞🏼

[tonyarnold]

The code is great, and well documented.

I've made a few suggestions about the wording of the documentation. "Unpredictable" isn't the word I would use to describe the project structures that don't work out of the box here - they're unexpected, perhaps?

Thanks for the hard work on this! Looking forward to seeing it merged 👍

[garricn]

The code is great, and well documented.

I've made a few suggestions about the wording of the documentation. "Unpredictable" isn't the word I would use to describe the project structures that don't work out of the box here - they're unexpected, perhaps?

Thanks for the hard work on this! Looking forward to seeing it merged 👍

Thank you for your contributions @tonyarnold. We're also looking forward to getting it merged.

[garricn]

Hello @jpsim @marcelofabri @SimplyDanny 😄

I'm reaching out to see if there is anything preventing this PR from being prioritized and merged. Please let me know what is needed and I will respond quickly, as we're eager to get this fix in. The latest release version of SwiftLintPlugin does not work for our use case as we rely on parent/child configurations and nested configurations. Please advise. Thank you 😄

[tinder-garricnahapetian]

Hello @jpsim @marcelofabri @SimplyDanny 👋

It's been a few months so I wanted to check in again. What are the next steps for this PR? Please let me know as we are eager to get it merged in. Thank you 😄

[garricn]

Hi @SimplyDanny. Thank you for approving and merging my other two recent PRs. May you please review and merge this PR as well 🙏 Let me know what you think. Thank you 😄

[tinder-cfuller]

@garricn This change is very much welcomed and appreciated. Thank you for submitting this!

I see that it has been open for 1 year. I would like to kindy mention the project maintainers, @jpsim @SimplyDanny and @marcelofabri, to please comment and provide guidance. I find this change to be ideal and would like to see this merged in. But ask the maintainers to please clarify why this change has not been been accepted yet. Does it need further changes? Or should the PR simply be closed at this point? Asking since knowing that it will not be merged is very helpful as well – so then I can focus on creating an alternative custom solution for the packages I maintain.

Thank you!

CC: @tonyarnold

[SimplyDanny]

First of all: Sorry for the very late response! Thank you for your patience and continuous reminders. The problem is that none of the maintainers actually uses the plugin. So we cannot really judge on these kinds of changes convincingly.

I'm fine with merging it once a few comprehensive questions are clarified and some nits are fixed.

It would be really great if you guys can jump in if any issues with the plugin come up in the future. With your experience you are more capable of answering question, provide some help or propose fixes.

[tinder-garricnahapetian]

First of all: Sorry for the very late response! Thank you for your patience and continuous reminders. The problem is that none of the maintainers actually uses the plugin. So we cannot really judge on these kinds of changes convincingly.

I'm fine with merging it once a few comprehensive questions are clarified and some nits are fixed.

It would be really great if you guys can jump in if any issues with the plugin come up in the future. With your experience you are more capable of answering question, provide some help or propose fixes.

@SimplyDanny Thank you so much! This is really great to hear. Please feel free to mention me on any future issues that arise with the plugin.

[tinder-cfuller]

Thank you @SimplyDanny! ❤️