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

Cannot invoke plugins from command line #483

Closed
lucteo opened this issue Feb 12, 2023 · 13 comments · Fixed by #608
Closed

Cannot invoke plugins from command line #483

lucteo opened this issue Feb 12, 2023 · 13 comments · Fixed by #608

Comments

@lucteo
Copy link

lucteo commented Feb 12, 2023

Hi

I'm trying to add swift-format to our project; we aim at being able to run this manually, and from our CI pipeline.
swift-format is added as a dependency in our Package.swift. (in my tests, I'm using 0.50800.0-SNAPSHOT-2022-12-29-a tag).

If I run "Lint Source Code" or "Format Source Code" from XCode, everything works ok.
However, If I try to run it from command line, I get the following errors:

$ swift package plugin lint-source-code
Building for debugging...
Build complete! (0.58s)
Error: '--recursive' is only valid when formatting or linting files
Usage: swift-format lint [--configuration <configuration>] [--assume-filename <assume-filename>] [--recursive] [--ignore-unparsable-files] [--parallel] [--color-diagnostics] [--no-color-diagnostics] [<paths> ...] [--strict]
  See 'swift-format lint --help' for more information.
error: swift-format invocation failed: NSTaskTerminationReason(rawValue: 1):64

Can you please help me make this work?

Thank you very much

@dabrahams
Copy link
Contributor

Lucian do you have a pointer to the package plugin you're working with?

@lucteo
Copy link
Author

lucteo commented Feb 15, 2023

The ones included in swift-format: https://github.com/apple/swift-format/tree/main/Plugins (in the above example, I'm using the linter plugin)

@dabrahams
Copy link
Contributor

I see, there are no tests for those in the project. Nice. You can use "swift package edit swift-format" and then go in and add printing to the place where the command is issued, to see what it's actually trying to do.

@dabrahams
Copy link
Contributor

dabrahams commented Feb 15, 2023

My guess is that this thing should not be trying to use --recursive at all. Its logic is very weird, such that it lints everything that lives in the same directory as any target file, whether it's a target or not. Also submits a directory for each target file, without uniquing them, which could be problematic depending on how SPM deals with it.

@macshome
Copy link

macshome commented Mar 1, 2023

When invoking the lint command in Xcode the plugin is being passed the project directory, hence the --recursive argument.

The lint plugin is hardcoded with "--recursive", "--parallel", "--strict" as arguments which isn't really what I expected.

The default of using --strict for the lint plugin causes it to exit with a 1 and always throw an error.

@macshome
Copy link

macshome commented Mar 2, 2023

I'm going to open a PR for the strict thing, but I also debugged the issues that @lucteo was running into.

The command plugins operate on a target, currently, not a directory. So you need to tell it what target to lint.

For example: swift package plugin lint-source-code --target swift-format

This mirrors the behavior of the Xcode plugin, if you want to run swift-format as a tool on a directory I would just call it directly.

@dabrahams
Copy link
Contributor

Maybe I'm misunderstanding the logic I described here, but by my reading it looks like it would seem to work in “normal” projects, but break in unusual projects, e.g. when there are non-target swift files in the same directory as Swift files. I also think you should reconsider whether it's really right to submit whole directories to swift-format.

@lucteo
Copy link
Author

lucteo commented Mar 2, 2023

I can confirm that adding --target to the command line makes the tool succeed. Thank you very much

Does this tool make sense to be run at the directory level?

(in any case, I would argue that we need to fix the tool to properly indicate the user the need of specifying a target)

@macshome
Copy link

macshome commented Mar 3, 2023

In the context of using a plugin though the target that is passed is the directory that the target code lives in. It doesn't seem to have any other way of passing a location. It also though only seems to read .swift files and there is an option to ignore non-parseable files.

When running as a standalone tool, swift-format seems to have more flexibility on the input.

@dabrahams
Copy link
Contributor

@macshome
Copy link

macshome commented Mar 3, 2023

Ahh, that helps immensely and I can get the source files now from the target.

When using --recursive though it's already building a file list of the .swift files in the project directory with the FileItereator utility.

Honestly I don't have a lot of use for the lint command plugin as the warnings don't show up in Xcode as they aren't build warnings. For that we need a BuildPlugin.

I'll make a new issue for the removal of --strict in the lint plugin default args and open a PR for it. The code isn't very testable for that sort of thing, but I'll try to figure something out.

For the scope of this particular ticket it seems that calling a Command Plugin without a target specified was the actual issue. I think that can probably be fixed with documentation on how to call a command plugin from the CLI.

@macshome
Copy link

macshome commented Mar 6, 2023

Opened PR #489 for the --strict thing.

@soumyamahunt
Copy link

(in any case, I would argue that we need to fix the tool to properly indicate the user the need of specifying a target)

@lucteo @macshome I would suggest the plugin should pick all the targets in the absence of specific targets. Instead of this:
https://github.com/apple/swift-format/blob/5bcfbff16e42ea22a697504db6f3b88e058535e6/Plugins/FormatPlugin/plugin.swift#L41

we should have this:

let targetsToFormat = targetNames.isEmpty ? context.package.targets : try context.package.targets(named: targetNames)

allevato added a commit that referenced this issue Sep 6, 2023
Default to all targets when plugin `--target` parameter missing. Fix #483
allevato added a commit to allevato/swift-format that referenced this issue Sep 14, 2023
…s-fix-483

Default to all targets when plugin `--target` parameter missing. Fix swiftlang#483
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 a pull request may close this issue.

4 participants