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

Fixes SwiftLint support on Xcode Cloud #4485

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Conversation

JagCesar
Copy link
Contributor

This PR fixes so SwiftLint installed as Xcode Plugin can be used on Xcode Cloud. The problem was that Xcode Cloud doesn't have write access to the .cachesDirectory. We solved this by always using NSTemporaryDirectory() if we're being run on a CI.

This will have the side-effect of doing the same on all CI's, but we couldn't come up with a way to distinguish Xcode Cloud from other CI's.

Solved this together with @westerlund

fixes #4484

@SwiftLintBot
Copy link

SwiftLintBot commented Oct 28, 2022

18 Messages
📖 Linting Aerial with this PR took 1.43s vs 1.42s on main (0% slower)
📖 Linting Alamofire with this PR took 2.06s vs 2.04s on main (0% slower)
📖 Linting Brave with this PR took 9.8s vs 9.78s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 4.11s vs 4.09s on main (0% slower)
📖 Linting Firefox with this PR took 9.05s vs 9.05s on main (0% slower)
📖 Linting Kickstarter with this PR took 14.91s vs 14.79s on main (0% slower)
📖 Linting Moya with this PR took 0.93s vs 0.95s on main (2% faster)
📖 Linting NetNewsWire with this PR took 4.16s vs 4.14s on main (0% slower)
📖 Linting Nimble with this PR took 0.91s vs 0.9s on main (1% slower)
📖 Linting PocketCasts with this PR took 14.95s vs 15.07s on main (0% faster)
📖 Linting Quick with this PR took 0.29s vs 0.28s on main (3% slower)
📖 Linting Realm with this PR took 17.17s vs 16.87s on main (1% slower)
📖 Linting SourceKitten with this PR took 0.56s vs 0.56s on main (0% slower)
📖 Linting Sourcery with this PR took 3.57s vs 3.56s on main (0% slower)
📖 Linting Swift with this PR took 6.34s vs 6.35s on main (0% faster)
📖 Linting VLC with this PR took 1.7s vs 1.71s on main (0% faster)
📖 Linting Wire with this PR took 17.66s vs 17.81s on main (0% faster)
📖 Linting WordPress with this PR took 13.99s vs 13.98s on main (0% slower)

Generated by 🚫 Danger

@@ -71,7 +71,14 @@ extension Configuration {

internal var cacheURL: URL {
let baseURL: URL
if let path = cachePath {

if ProcessInfo.processInfo.environment["CI"] == "TRUE" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CI is a super generic environment variable name. Is there a more reliable way to detect if this is running in an Xcode Cloud environment? I won't want everyone running SwiftLint in their CI to suddenly lose caching.

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Calling NSTemporaryDirectory() will return a new directory every time, effectively disabling caching of linter results. If we do something like this, it should be by disabling caching explicitly.

@westerlund
Copy link

Calling NSTemporaryDirectory() will return a new directory every time, effectively disabling caching of linter results. If we do something like this, it should be by disabling caching explicitly.

Yeah we thought of that and the problem is that we can't send any parameters to Xcode Plugins (as you know) and the only other solution would be to introduce another plugin target which explicitly disables caching, what do you think? Is it better to introduce a second SwiftLintPlugin or disable caching entirely only on Xcode Cloud?

@jpsim
Copy link
Collaborator

jpsim commented Oct 31, 2022

Does Xcode Cloud have any way to persist state across CI jobs at all? If not, then we should find the most reliable way to detect we're running on that platform and disable caching completely.

@jpsim
Copy link
Collaborator

jpsim commented Oct 31, 2022

If not, then we should find the most reliable way to detect we're running on that platform and disable caching completely.

Here: https://github.com/realm/SwiftLint/blob/0.50.0-rc.3/Source/swiftlint/Commands/Lint.swift#L53

Could be

ignoreCache: noCache || ProcessInfo.processInfo.environment["SWIFTLINT_DISABLE_CACHE"] == "TRUE"

@westerlund
Copy link

Does Xcode Cloud have any way to persist state across CI jobs at all? If not, then we should find the most reliable way to detect we're running on that platform and disable caching completely.

In theory DerivedData will be cached between builds, however it seems like we don't have write access to that path.

If not, then we should find the most reliable way to detect we're running on that platform and disable caching completely.

Here: https://github.com/realm/SwiftLint/blob/0.50.0-rc.3/Source/swiftlint/Commands/Lint.swift#L53

Could be

ignoreCache: noCache || ProcessInfo.processInfo.environment["SWIFTLINT_DISABLE_CACHE"] == "TRUE"

I did some digging and it seems like Xcode Cloud is the only CI setting the variable CI_BUNDLE_ID, so expanding on your suggestion it could be something along the lines of:

ignoreCache: noCache || ProcessInfo.processInfo.environment["CI_BUNDLE_ID"] != nil

We'd much rather have this working automatically than manually setting an environment value on the Target itself, what do you think? Would it be reliable enough? At least until we get write permissions on the Xcode Cloud VM.

@jpsim
Copy link
Collaborator

jpsim commented Nov 1, 2022

CI_BUNDLE_ID is super generic too and possible that either some other CI or even user's dev machines have that set for some reason, and then they'd lose SwiftLint caching. So I'm not comfortable disabling caching based on the presence of that environment variable.

Xcode Cloud supports setting custom environment variables, so I'd like to propose that users define the SWIFTLINT_DISABLE_CACHE variable and we can check for it in SwiftLint.

In addition to predefined environment variables, set custom environment variables in a workflow’s Environment section and access them in your custom build scripts or test actions.

@westerlund
Copy link

CI_BUNDLE_ID is super generic too and possible that either some other CI or even user's dev machines have that set for some reason, and then they'd lose SwiftLint caching. So I'm not comfortable disabling caching based on the presence of that environment variable.

Xcode Cloud supports setting custom environment variables, so I'd like to propose that users define the SWIFTLINT_DISABLE_CACHE variable and we can check for it in SwiftLint.

In addition to predefined environment variables, set custom environment variables in a workflow’s Environment section and access them in your custom build scripts or test actions.

Alright, I'll update the PR and also update the README to reflect this change. Thanks for the input!

@jpsim
Copy link
Collaborator

jpsim commented Nov 1, 2022

You know what, looking at the Xcode Cloud docs again, I think we can be reasonably sure that we're running on Xcode Cloud if all of the following environment variables are set:

CI
CI_BUILD_ID
CI_BUILD_NUMBER
CI_BUNDLE_ID
CI_COMMIT
CI_DERIVED_DATA_PATH
CI_PRODUCT
CI_PRODUCT_ID
CI_PRODUCT_PLATFORM
CI_PROJECT_FILE_PATH
CI_START_CONDITION
CI_TEAM_ID
CI_WORKFLOW
CI_WORKSPACE
CI_XCODE_PROJECT
CI_XCODE_SCHEME
CI_XCODEBUILD_ACTION
CI_XCODEBUILD_EXIT_CODE

So you could extract a function that determines if we're running on Xcode Cloud.

If someone or some other CI sets all the same environment variables, then I think it's reasonable to disable caching.

@westerlund
Copy link

You know what, looking at the Xcode Cloud docs again, I think we can be reasonably sure that we're running on Xcode Cloud if all of the following environment variables are set:

CI
CI_BUILD_ID
CI_BUILD_NUMBER
CI_BUNDLE_ID
CI_COMMIT
CI_DERIVED_DATA_PATH
CI_PRODUCT
CI_PRODUCT_ID
CI_PRODUCT_PLATFORM
CI_PROJECT_FILE_PATH
CI_START_CONDITION
CI_TEAM_ID
CI_WORKFLOW
CI_WORKSPACE
CI_XCODE_PROJECT
CI_XCODE_SCHEME
CI_XCODEBUILD_ACTION
CI_XCODEBUILD_EXIT_CODE

So you could extract a function that determines if we're running on Xcode Cloud.

If someone or some other CI sets all the same environment variables, then I think it's reasonable to disable caching.

Good idea, we'll go ahead and fix that. I'm glad we could come up with a solution that disables caching automatically , because then we can remove it later when the environment grants us write access!

Ignore my commits, I'll get back with the fix later!

I think it's reasonable to have a section in the README about caching not working on Xcode Cloud, what do you think? I also saw that the README is available in a few other languages than English, how do we properly update all of them?

@jpsim
Copy link
Collaborator

jpsim commented Nov 1, 2022

I think it's reasonable to have a section in the README about caching not working on Xcode Cloud, what do you think? I also saw that the README is available in a few other languages than English, how do we properly update all of them?

I don't think this needs a readme entry, and when we update the readme we typically only update the English one, the other ones are occasionally updated by other contributors who speak that language.

Fixes realm#4484 by checking the process environment for all the expected
environment variables set by Xcode Cloud.
@jpsim
Copy link
Collaborator

jpsim commented Nov 1, 2022

Looks good @JagCesar, thanks for the PR.

I made a few small modifications and will be merging once CI is 🟢

  • Moved the check so it also applies when running analyze
  • Updated ProcessInfo.isLikelyXcodeCloudEnvironment to only access environment once, in case there's any overhead involved, so it's not done repeatedly.

@jpsim jpsim enabled auto-merge (squash) November 1, 2022 16:34
@jpsim jpsim merged commit c56e19a into realm:main Nov 1, 2022
slavikus added a commit to evehome/SwiftLint that referenced this pull request Oct 18, 2023
…e Cloud

In Xcode Cloud environment, SwiftLint’s cache cannot be written. When using the SwiftLinkPlugin, there is no way to disable the cache. Previously, a solution was made for the SwiftLint CLI itself where it looks at a set of environment variables (realm#4485). This solution offers a cleaner approach where the plugin itself decides whether it needs to enable or disable the cache based on the `CI_XCODE_CLOUD` environment variable.
SimplyDanny pushed a commit that referenced this pull request Nov 5, 2023
)

In Xcode Cloud environment, SwiftLint’s cache cannot be written. When using the SwiftLintPlugin, there is no way to disable the cache. Previously, a solution was made for the SwiftLint CLI itself where it looks at a set of environment variables (#4485). This solution offers a cleaner approach where the plugin itself decides whether it needs to enable or disable the cache based on the `CI_XCODE_CLOUD` environment variable.

In the long run, SwiftLint should get rid of `isLikelyXcodeCloudEnvironment` entirely. The caller should always decide if caching is possible or not, while SwiftLint itself should be platform-agnostic.
u-abyss pushed a commit to u-abyss/SwiftLint that referenced this pull request Dec 16, 2023
…alm#5287)

In Xcode Cloud environment, SwiftLint’s cache cannot be written. When using the SwiftLintPlugin, there is no way to disable the cache. Previously, a solution was made for the SwiftLint CLI itself where it looks at a set of environment variables (realm#4485). This solution offers a cleaner approach where the plugin itself decides whether it needs to enable or disable the cache based on the `CI_XCODE_CLOUD` environment variable.

In the long run, SwiftLint should get rid of `isLikelyXcodeCloudEnvironment` entirely. The caller should always decide if caching is possible or not, while SwiftLint itself should be platform-agnostic.
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.

SwiftLint as Xcode Plugin doesn't work on Xcode Cloud
4 participants