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

Update SwiftLint plugin to use `environment` property on `Process` #207

Merged
merged 13 commits into from Mar 8, 2019

Conversation

Projects
None yet
4 participants
@JosephDuffy
Copy link
Contributor

JosephDuffy commented Mar 7, 2019

This was failing when the system shell is fish (because fish requires env to be prepended). Using the environment property on Process makes this shell-agnostic

Update SwiftLint plugin to use `environment`
This was failing when the system shell is fish. Using the `environment` property on `Process` makes this shell-agnostic
import Logger
@testable import RunnerLib
import XCTest

This comment has been minimized.

@JosephDuffy

JosephDuffy Mar 7, 2019

Author Contributor

These changes seem to have been done during the pre-commit hook

@@ -42,6 +42,13 @@ extension DangerfilePathFinderTests {
]
}

extension GetDangerJSPathTests {

This comment has been minimized.

@JosephDuffy

JosephDuffy Mar 7, 2019

Author Contributor

These changes seem to have been done during the pre-commit hook

@JosephDuffy

This comment has been minimized.

Copy link
Contributor Author

JosephDuffy commented Mar 7, 2019

This was working locally but isn't in CI. Will fix!

@f-meloni
Copy link
Member

f-meloni left a comment

Looks good can you please update the changelog?

@f-meloni
Copy link
Member

f-meloni left a comment

Looks there are some problems with CI on linux

@JosephDuffy

This comment has been minimized.

Copy link
Contributor Author

JosephDuffy commented Mar 7, 2019

Looks good can you please update the changelog?

Thanks! I've updated the changelog with the change

@JosephDuffy

This comment has been minimized.

Copy link
Contributor Author

JosephDuffy commented Mar 7, 2019

Looks there are some problems with CI on linux

I can't immediately see what's going wrong here, I'll look in to it further

JosephDuffy added some commits Mar 7, 2019

Fix SwiftLint warnings
Maybe these were causing the linux builds to fail on CI? The reason isn’t obvious to me
Remove leading whitespace
This was done during the pre-commit hook but didn’t seem to be committed
@orta

This comment has been minimized.

Copy link
Member

orta commented Mar 8, 2019

This is very weird:

danger:runDangerSubprocess Passed DSL in via STDIN +0ms
The program 'swift' is currently not installed. To run 'swift' please ask your administrator to install the package 'python-swiftclient'
Ran with: /tmp/_tmp_dangerfile.swift /tmp/danger-dsl.json /tmp/danger-response.json
Decoding the DSL into Swift types
Setting up to dump results
Executing `swift run swiftlint lint --quiet --reporter json --use-script-input-files --force-exclude` with environment variables ["SCRIPT_INPUT_FILE_1": "Sources/Danger/Plugins/SwiftLint/ShellExecutor.swift", "SCRIPT_INPUT_FILE_2": "Sources/Danger/Plugins/SwiftLint/SwiftLint.swift", "SCRIPT_INPUT_FILE_COUNT": "3", "SCRIPT_INPUT_FILE_0": "Sources/Danger-Swift/Fake.swift"]
Sending results back to Danger
@f-meloni

This comment has been minimized.

Copy link
Member

f-meloni commented Mar 8, 2019

Could it be related to the env override? Maybe it has to be merged with the current process env?
(The env of a process is inherited from the current process)
Will make a try to see if it fixes it

@f-meloni

This comment has been minimized.

Copy link
Member

f-meloni commented Mar 8, 2019

Ok fixed! 🎉
Can you please use the same logic on spawn, and remove the test logs? And we are ready to merge :)

@JosephDuffy

This comment has been minimized.

Copy link
Contributor Author

JosephDuffy commented Mar 8, 2019

Could it be related to the env override? Maybe it has to be merged with the current process env?
(The env of a process is inherited from the current process)
Will make a try to see if it fixes it

Ah I hadn't thought of this, good catch!

I should have a chance to clean this up tonight/tomorrow.

Thanks for the fix 😄

JosephDuffy added some commits Mar 8, 2019

Add comment to Fake.swift
This file is in a cycle of changes, I think thanks to SwiftLint
It seems to want to end with a new line, but also not start with whitespace. This comment both explains the file and fixes the SwiftLint issue
@DangerCI

This comment has been minimized.

Copy link

DangerCI commented Mar 8, 2019

Warnings
⚠️

Sources/Danger/Plugins/SwiftLint/ShellExecutor.swift#L3 - TODOs should be resolved (Get a logger into here this is...). (todo)

Generated by 🚫 Danger Swift against ebb5189

arguments.joined(separator: " ")].filter { !$0.isEmpty }.joined(separator: " ")
print("Executing \(script)")
print("Executing `\(script)` with environment variables \(environmentVariables)")

This comment has been minimized.

@JosephDuffy

JosephDuffy Mar 8, 2019

Author Contributor

@f-meloni this was here before, but I have updated it to include the environment variables now.

Should I remove this or keep it?

If I keep I'll update it to print the merged envs and also add it to the spawn function below?

This comment has been minimized.

@f-meloni

f-meloni Mar 8, 2019

Member

Yes, I like it :)

Refactor logic for created task to function
`print` statement was also removed. This may we re-added in response to https://github.com/danger/swift/pull/207/files#r263776454
@@ -0,0 +1 @@
// An empty file to allow the DangerDependencies target to build via SPM

This comment has been minimized.

@f-meloni

f-meloni Mar 8, 2019

Member

Thanks :)

@f-meloni

This comment has been minimized.

Copy link
Member

f-meloni commented Mar 8, 2019

merge on green

@peril-staging peril-staging bot merged commit db863cd into danger:master Mar 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.