Check if swiftformat is installed in scripts/soundness.sh

natikgadzhi commented 1 year ago

This pull request adds a check for swiftformat executable in scripts/soundness.sh so that it doesn't fail silently if the system does not have swiftformat installed.

Motivation:

I was working on #133 and noticed that soundness passes locally, but fails on CI. Turned out, it just didn't work locally because I didn't have swiftformat installed.

This pull request adds a check that would exit 1 from soundness if swiftformat is not an executable. /cc @ktoso

ktoso commented 1 year ago

Hmmm, I'm surprised we don't fail but it seems we don't set -x so yeah it wouldn't...

WDYT @tomerd worth adding this or setting -x so any error fails out the script?

natikgadzhi commented 1 year ago

I just realized that:

Several SSWG projects have soundness.sh check that would run swiftformat. For example, here's one in swift-log.
There's an issue to update swwiftformat in swift-log, which probably makes sense to update and enforce across all/most sswg projects https://github.com/apple/swift-log/issues/239

Perhaps that soundness check could be extracted into a separate GitHub action, and used across repos, so we're sure the same checks are running? swiftformat version can be configurable. Want me to throw one together?

ktoso commented 1 year ago

We'd love to have a swiftpm plugin which does the job of the script and then share that actually; I think someone was working on that @FranzBusch do you remember maybe?

FranzBusch commented 1 year ago

@glbrntt prototyped something a while ago https://github.com/apple/swift-nio/pull/2242. There are a bunch of open questions like where should it live, how configurable it should be, etc. .

natikgadzhi commented 1 year ago

Looks like https://github.com/apple/swift-nio/pull/2242 focuses on licenses, and doesn't explicitly address formatting (NIO does not have swiftformat step in their soundness check as of that PR). But, consolidating the whole soundness check, or making swift-format available as a package manager plugin sounds like a good idea.

I just realized there's SwiftFormat and swift-format. The former already has a Swift Package Manager plugin (good!), and that's the one that is used in swift-metrics.

On it's own, moving one check from soundness.sh to adding a dependency and running swift run swiftformat in a single repository doesn't add too much value.

Would it be a better idea to:

Make a separate package, i.e. swift/contrib-validator that would:
- include checks from https://github.com/apple/swift-nio/pull/2242
- include swiftformat plugin OR swift-format plugin (we'd need to add that, but that should not be hard).
- be configurable with .contrib-validations.yaml or something similar to toggle checks on/off
Add that package as a plugin dependency to swift-metrics and other SSWG projects gradually, replacing their soundness checks that are copied over.

How does that sound? (no pun intended).

FranzBusch commented 1 year ago

Something that does all of our soundness features in one would be great. However, we do have problem where we should put such a thing. We can't add arbitrary dependencies to NIO and our other repositories. That's also the reason why the current effort was put on hold.

natikgadzhi commented 1 year ago

@FranzBusch, understood. I see that NIO has a very minimal list of dependencies, limited to infrastructure packages (atomics and collections).

There are also apple/swift-tools-support-async and apple/swift-tools-support-core used in llbuild2 (and perhaps other spots), but unlike atomics and collections, these two seem to be primarily intended for internal use in other Apple libraries.

Would it be a good idea to have a special little repo with that plugin, say apple/swift-soundness-check?

I'd assume the problem is not so much in writing the plugin initially, but in figuring out who should own maintenance and future improvements for it, and who should support all other libraries switching to it? /cc @ktoso

FranzBusch commented 1 year ago

@natikgadzhi You are correct in that the problem is not about writing the actual plugin but actually where to put it and if and how we can depend on it from the other repos. This is something we have to figure out internally and until then we have to live with the current shell scripts. Once, we have a solution for the organisational problems we can move forward with this.

This is not blocking this PR here though!

natikgadzhi commented 1 year ago

@FranzBusch, thank you for the explanation!

Yep, then the question is, should we also add set -x so the script becomes more verbose, or should we just take the little improvement.

I think set -x will make things more verbose and contributor-friendly, but also somewhat brake the formatting of output that the script has now:

swift-metrics feature/duration*
❯ ./scripts/soundness.sh
+++ dirname ./scripts/soundness.sh
++ cd ./scripts
++ pwd
+ here=/Users/nategadzhi/src/apple/swift-metrics/scripts
+ printf '=> Checking linux tests... '
=> Checking linux tests... ++ git status --porcelain
+ FIRST_OUT=' M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh'
+ ruby /Users/nategadzhi/src/apple/swift-metrics/scripts/../scripts/generate_linux_tests.rb
++ git status --porcelain
+ SECOND_OUT=' M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh'
+ [[  M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh != \ \M\ \S\o\u\r\c\e\s\/\C\o\r\e\M\e\t\r\i\c\s\/\M\e\t\r\i\c\s\.\s\w\i\f\t\
\ \M\ \S\o\u\r\c\e\s\/\M\e\t\r\i\c\s\T\e\s\t\K\i\t\/\T\e\s\t\M\e\t\r\i\c\s\.\s\w\i\f\t\
\ \M\ \s\c\r\i\p\t\s\/\s\o\u\n\d\n\e\s\s\.\s\h ]]
+ printf '\033[0;32mokay.\033[0m\n'
okay.
+ printf '=> Checking for unacceptable language... '
=> Checking for unacceptable language... + unacceptable_terms=(-e blacklis[t] -e whitelis[t] -e slav[e] -e sanit[y])
+ git grep --color=never -i -e 'blacklis[t]' -e 'whitelis[t]' -e 'slav[e]' -e 'sanit[y]'
+ printf '\033[0;32mokay.\033[0m\n'
okay.
+ printf '=> Checking format... '
=> Checking format... ++ which swiftformat
+ [[ ! -x /opt/homebrew/bin/swiftformat ]]
++ git status --porcelain
+ FIRST_OUT=' M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh'
+ swiftformat .
++ git status --porcelain
+ SECOND_OUT=' M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh'
+ [[  M Sources/CoreMetrics/Metrics.swift
 M Sources/MetricsTestKit/TestMetrics.swift
 M scripts/soundness.sh != \ \M\ \S\o\u\r\c\e\s\/\C\o\r\e\M\e\t\r\i\c\s\/\M\e\t\r\i\c\s\.\s\w\i\f\t\
\ \M\ \S\o\u\r\c\e\s\/\M\e\t\r\i\c\s\T\e\s\t\K\i\t\/\T\e\s\t\M\e\t\r\i\c\s\.\s\w\i\f\t\
\ \M\ \s\c\r\i\p\t\s\/\s\o\u\n\d\n\e\s\s\.\s\h ]]
+ printf '\033[0;32mokay.\033[0m\n'
okay.

And so on. My take is that this level of output is too verbose, and makes it hard to read.

What about set -o pipefail instead, @ktoso, @tomerd?

natikgadzhi commented 1 year ago

@ktoso, um, guys. I messed up and included the soundness piece in the previous PR I worked on (#133). So this one automatically closed.

I think set -o pipefail is a good idea, but I won't nudge you about it too much.

tomerd commented 1 year ago

I think set -o pipefail is a good idea, but I won't nudge you about it too much.

PR welcome

apple / swift-metrics

Check if swiftformat is installed in scripts/soundness.sh #134

Motivation: