Skip to content
This repository has been archived by the owner. It is now read-only.

Add nuclide-swift #621

Closed
wants to merge 1 commit into from
Closed

Conversation

@modocache
Copy link
Contributor

modocache commented Jul 20, 2016

Swift package manager integration with Nuclide. Build Swift packages, run their test suites, and view the results.

Future commits will have this package parse the YAML build output from Swift package manager in order to provide accurate autocompletion, type hints, and more. See http://modocache.io/nuclide-swift-ide for details.

Some screenshots:

The toolbar, task name "build"
The toolbar dropdown
The build settings
The toolbar, task name "test"
The test settings

@ghost ghost added the CLA Signed label Jul 20, 2016
}));
}

export function consumeOutputService(service: OutputService): void {

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 20, 2016

Contributor

Last week I made it so that the task runner package will call setProjectRoot() on task runners (if that method exists) so you don't really need to consume this service.

This comment has been minimized.

Copy link
@modocache

modocache Jul 21, 2016

Author Contributor

Yeah, I saw that API -- neat! Did you mean to leave this comment on the consumeCurrentWorkingDirectory line, though? I think I'll need the output service, to display the build output from SwiftPM.


const dispatcher = new Dispatcher();
this._store = new SwiftPMTaskRunnerStore(dispatcher, initialState);
this._actions = new SwiftPMTaskRunnerActions(dispatcher);

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 20, 2016

Contributor

Do you think you could lazily create these when they're used instead? Since we're using inline imports, that would save us having to do the IO for loading the modules until/unless the user actually selected a Swift task.

This comment has been minimized.

Copy link
@modocache

modocache Jul 21, 2016

Author Contributor

Ah, that explains why nuclide-buck creates these lazily. Will do.

this._logOutput(message.data, 'log');
break;
case 'exit':
this._logOutput(`Exited with exit code ${message.exitCode}`, 'log');

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 20, 2016

Contributor

How about using "success" and "error" levels for 0/non-zero exit codes?

This comment has been minimized.

Copy link
@modocache

modocache Jul 21, 2016

Author Contributor

That makes so much sense...! Ha, completely spaced on that. Thanks, will do!

This comment has been minimized.

Copy link
@modocache

modocache Jul 21, 2016

Author Contributor

Wow, that looks awesome. Thanks!

screen shot 2016-07-20 at 9 38 14 pm

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from be8077c to ea72c51 Jul 21, 2016
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 21, 2016

@modocache updated the pull request.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from ea72c51 to a744518 Jul 21, 2016
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 21, 2016

@modocache updated the pull request.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from a744518 to 1a8371e Jul 21, 2016
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 21, 2016

@modocache updated the pull request.

@modocache

This comment has been minimized.

Copy link
Contributor Author

modocache commented Jul 21, 2016

@matthewwithanm Thanks! I think I've addressed all your feedback. Give this another look when you can.

}

observeTaskList(callback: (taskList: Array<TaskMetadata>) => mixed): IDisposable {
if (this._taskList == null) {

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 21, 2016

Contributor

Since the task meta doesn't actually change, you could just do:

callback(SwiftPMTaskRunnerTaskMetadata);
return new Disposable();

nbd though.


getExtraUi(): ReactClass<any> {
const {store, actions} = this._getFlux();
return class ExtraUi extends React.Component {

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 21, 2016

Contributor

Hm, I think I'm actually calling this several times. I guess I should change that!


setProjectRoot(projectRoot: ?Directory): void {
const path = projectRoot == null ? '' : projectRoot.getPath();
fsPromise.exists(`${path}/Package.swift`).then((fileExists) => {

This comment has been minimized.

Copy link
@matthewwithanm

matthewwithanm Jul 21, 2016

Contributor

nit: I don't think this will lint. (We avoid parens for single arg arrow functions.)

Also, what's the desired behavior for this when your current project isn't a swift one? Not sure what all this would affect, but it seems a little weird for the directory to "stick" to the last selected swift project.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from 1a8371e to f2f00ac Jul 21, 2016
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 21, 2016

@modocache updated the pull request.

@modocache

This comment has been minimized.

Copy link
Contributor Author

modocache commented Jul 21, 2016

Also, what's the desired behavior for this when your current project isn't a swift one? Not sure what all this would affect, but it seems a little weird for the directory to "stick" to the last selected swift project.

@matthewwithanm I thought of a few alternatives:

  1. Disable/gray-out the task runner toolbar when the project root is set to a path that doesn't contain a Package.swift file.
  2. Update based on the current project root no matter what. Instead of attempting to guess what is and isn't a Swift package, we allow the task runner toolbar to attempt to build/test anyway.

I don't mind the current "sticky" behavior, but if either of the above is more common in Nuclide, I'll implement that.

@matthewwithanm

This comment has been minimized.

Copy link
Contributor

matthewwithanm commented Jul 23, 2016

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 23, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@zertosh

This comment has been minimized.

Copy link
Contributor

zertosh commented Jul 24, 2016

When we tried to land this, it failed the flow check, and the "nuclide deactivates cleanly" integration test (and will fail the lint check). First, rebase past 8632e79 (just landed). Then:

  • In SwiftPMTaskRunnerToolbar.js fix the lint warnings. We treat all lint issues as warnings so we can distinguish them from flow errors, but they're both land blocking. You need:
diff --git a/pkg/nuclide-swift/lib/taskrunner/toolbar/SwiftPMTaskRunnerToolbar.js b/pkg/nuclide-swift/lib/taskrunner/toolbar/SwiftPMTaskRunnerToolbar.js
index 92801b3..abeb2b0 100644
--- a/pkg/nuclide-swift/lib/taskrunner/toolbar/SwiftPMTaskRunnerToolbar.js
+++ b/pkg/nuclide-swift/lib/taskrunner/toolbar/SwiftPMTaskRunnerToolbar.js
@@ -14,7 +14,7 @@ import {AtomInput} from '../../../../nuclide-ui/lib/AtomInput';
 import {Button, ButtonSizes} from '../../../../nuclide-ui/lib/Button';
 import {
   SwiftPMTaskRunnerBuildTaskMetadata,
-  SwiftPMTaskRunnerTestTaskMetadata
+  SwiftPMTaskRunnerTestTaskMetadata,
 } from '../SwiftPMTaskRunnerTaskMetadata';
 import SwiftPMBuildSettingsModal from './SwiftPMBuildSettingsModal';
 import SwiftPMTestSettingsModal from './SwiftPMTestSettingsModal';
@@ -29,7 +29,7 @@ export default class SwiftPMTaskRunnerToolbar extends React.Component {
   }

   render(): React.Element<any> {
-    let settingsElements = [];
+    const settingsElements = [];
     switch (this.props.activeTaskType) {
       case SwiftPMTaskRunnerBuildTaskMetadata.type:
         settingsElements.push(
@@ -42,7 +42,7 @@ export default class SwiftPMTaskRunnerToolbar extends React.Component {
             onDismiss={() => this._hideSettings()}
             onSave={(configuration, Xcc, Xlinker, Xswiftc, buildPath) =>
               this._saveBuildSettings(configuration, Xcc, Xlinker, Xswiftc, buildPath)}
-          />
+          />,
         );
         break;
       case SwiftPMTaskRunnerTestTaskMetadata.type:
@@ -51,7 +51,7 @@ export default class SwiftPMTaskRunnerToolbar extends React.Component {
             buildPath={this.props.store.getTestBuildPath()}
             onDismiss={() => this._hideSettings()}
             onSave={buildPath => this._saveTestSettings(buildPath)}
-          />
+          />,
         );
         break;
       default:
@@ -106,7 +106,7 @@ export default class SwiftPMTaskRunnerToolbar extends React.Component {
       Xcc,
       Xlinker,
       Xswiftc,
-      buildPath
+      buildPath,
     );
     this._hideSettings();
   }
  • In SwiftPMTaskRunnerCommands.js update the path to featureConfig (I just moved it last night). This fixes one of the flow errors. You need:
diff --git a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunnerCommands.js b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunnerCommands.js
index db0c8aa..858b365 100644
--- a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunnerCommands.js
+++ b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunnerCommands.js
@@ -9,7 +9,7 @@
  * the root directory of this source tree.
  */

-import featureConfig from '../../../nuclide-feature-config';
+import featureConfig from '../../../commons-atom/featureConfig';

 export function buildCommand(
   chdir: string,
  • In SwiftPMTaskRunner.js wrap _outputMessages (a Subject) in a DisposableSubscription when adding it to _disposables. Subscriptions don't implement a dispose. DisposableSubscription turns a subscription into a disposable. This fixes another flow error, and the clean deactivation. You need:
diff --git a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
index 42760f6..6b35c9f 100644
--- a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
+++ b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
@@ -16,6 +16,7 @@ import type {SwiftPMTaskRunnerStoreState} from './SwiftPMTaskRunnerStoreState';
 import invariant from 'assert';
 import {Observable, Subject} from 'rxjs';
 import {Dispatcher} from 'flux';
+import {DisposableSubscription} from '../../../commons-node/stream';
 import {CompositeDisposable, Disposable} from 'atom';
 import {React} from 'react-for-atom';
 import fsPromise from '../../../commons-node/fsPromise';
@@ -68,7 +69,7 @@ export class SwiftPMTaskRunner {
     this._initialState = initialState;
     this._disposables = new CompositeDisposable();
     this._outputMessages = new Subject();
-    this._disposables.add(this._outputMessages);
+    this._disposables.add(new DisposableSubscription(this._outputMessages));
   }

   dispose(): void {
  • Fix the remaining flow errors. Uncomment the line in .flowconfig that references $FlowFB. Then you can flow w/o any of the missing internal module errors.
@zertosh

This comment has been minimized.

Copy link
Contributor

zertosh commented Jul 24, 2016

@matthewwithanm these are the remaining flow errors. idk what they're about.

kg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 22: export function observableToTaskInfo(observable: Observable<TaskEvent>): Task {
                                                                 ^^^^^^^^^ property `progress`. Property not found in. See: pkg/commons-node/observableToTaskInfo.js:22
 12: export type ProcessMessage = {
                                  ^ object type. See: pkg/commons-node/process-types.js:12

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 22: export function observableToTaskInfo(observable: Observable<TaskEvent>): Task {
                                                                 ^^^^^^^^^ property `progress`. Property not found in. See: pkg/commons-node/observableToTaskInfo.js:22
 15: } | {
         ^ object type. See: pkg/commons-node/process-types.js:15

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 22: export function observableToTaskInfo(observable: Observable<TaskEvent>): Task {
                                                                 ^^^^^^^^^ property `progress`. Property not found in. See: pkg/commons-node/observableToTaskInfo.js:22
 18: } | {
         ^ object type. See: pkg/commons-node/process-types.js:18

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 22: export function observableToTaskInfo(observable: Observable<TaskEvent>): Task {
                                                                 ^^^^^^^^^ property `progress`. Property not found in. See: pkg/commons-node/observableToTaskInfo.js:22
 21: } | {
         ^ object type. See: pkg/commons-node/process-types.js:21

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 13:   kind: 'stdout',
             ^^^^^^^^ string literal `stdout`. Expected string literal `progress`, got `stdout` instead. See: pkg/commons-node/process-types.js:13
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. See: pkg/nuclide-task-runner/lib/types.js:32

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 16:   kind: 'stderr',
             ^^^^^^^^ string literal `stderr`. Expected string literal `progress`, got `stderr` instead. See: pkg/commons-node/process-types.js:16
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. See: pkg/nuclide-task-runner/lib/types.js:32

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 19:   kind: 'exit',
             ^^^^^^ string literal `exit`. Expected string literal `progress`, got `exit` instead. See: pkg/commons-node/process-types.js:19
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. See: pkg/nuclide-task-runner/lib/types.js:32

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 22:   kind: 'error',
             ^^^^^^^ string literal `error`. Expected string literal `progress`, got `error` instead. See: pkg/commons-node/process-types.js:22
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. See: pkg/nuclide-task-runner/lib/types.js:32

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. Expected string literal `stdout`, got `progress` instead. See: pkg/nuclide-task-runner/lib/types.js:32
 13:   kind: 'stdout',
             ^^^^^^^^ string literal `stdout`. See: pkg/commons-node/process-types.js:13

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. Expected string literal `stderr`, got `progress` instead. See: pkg/nuclide-task-runner/lib/types.js:32
 16:   kind: 'stderr',
             ^^^^^^^^ string literal `stderr`. See: pkg/commons-node/process-types.js:16

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. Expected string literal `exit`, got `progress` instead. See: pkg/nuclide-task-runner/lib/types.js:32
 19:   kind: 'exit',
             ^^^^^^ string literal `exit`. See: pkg/commons-node/process-types.js:19

pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js:139
139:     const observable = observeProcess(
                            ^ call of method `do`
 32:   kind: 'progress',
             ^^^^^^^^^^ string literal `progress`. Expected string literal `error`, got `progress` instead. See: pkg/nuclide-task-runner/lib/types.js:32
 22:   kind: 'error',
             ^^^^^^^ string literal `error`. See: pkg/commons-node/process-types.js:22


Found 12 errors
zsh: exit 2     flow
@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from f2f00ac to 2ed934f Jul 24, 2016
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 24, 2016

@modocache updated the pull request.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from 2ed934f to 238f286 Jul 24, 2016
@modocache

This comment has been minimized.

Copy link
Contributor Author

modocache commented Jul 24, 2016

Thanks for the tips! I pushed up some changes to fix the Flow and linter errors. I silenced the observableToTaskInfo errors as well, with the following change:

diff --git a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
index 42760f6..856cd0a 100644
--- a/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
+++ b/pkg/nuclide-swift/lib/taskrunner/SwiftPMTaskRunner.js
@@ -137,7 +138,7 @@ export class SwiftPMTaskRunner {

     const observable = observeProcess(
       () => safeSpawn(command.command, command.args),
-    ).do(message => {
+    ).switchMap(message => {
       switch (message.kind) {
         case 'stderr':
         case 'stdout':
@@ -159,7 +160,7 @@ export class SwiftPMTaskRunner {
         default:
           break;
       }
-      return message;
+      return Observable.empty();
     });

     const task = observableToTaskInfo(observable);

Not sure if that's right, but it seems to work. :)

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 24, 2016

@modocache updated the pull request.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from 238f286 to 207e7d2 Jul 24, 2016
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 24, 2016

@modocache updated the pull request.

@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from 207e7d2 to 70a8f7c Jul 24, 2016
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 24, 2016

@modocache updated the pull request.

@jamesgpearce

This comment has been minimized.

Copy link
Contributor

jamesgpearce commented Jul 24, 2016

🎉

Swift package manager integration with Nuclide. Build Swift packages, run their
test suites, and view the results.

Future commits will have this package parse the YAML build output from Swift
package manager in order to provide accurate autocompletion, type hints, and
more.
@modocache modocache force-pushed the modocache:nuclide-swift-pr branch from 70a8f7c to a5aecb3 Jul 26, 2016
@ghost

This comment has been minimized.

Copy link

ghost commented Jul 26, 2016

@modocache updated the pull request.

@zertosh

This comment has been minimized.

Copy link
Contributor

zertosh commented Jul 26, 2016

@facebook-github-bot shipit

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 26, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost closed this in 2898a42 Jul 26, 2016
@zertosh

This comment has been minimized.

Copy link
Contributor

zertosh commented Jul 26, 2016

c1d7bbd9f368a080

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.