Skip to content

Commit

Permalink
Update RLS index after Buck build
Browse files Browse the repository at this point in the history
After hitting Buck build button, the relevant save-analysis Buck build
is performed and RLS is supplied with a dummy build command to retrieve
necessary save-analysis indexing files.

Things left to do/worth noting:
* We should detect if Buck is an appropriate build system for a given
LSP service and if not, not override RLS' external build command
* Until path remapping lands in rustc
(rust-lang/rust#53110) the index-based features
(find refs, goto def etc.) only work for source files in the
buck-out/**/$TARGET#save-analysis-container source files.
* RLS needs some bugfixin' since currently external build command does
not properly refresh file dirty state, leading to an endless build loop.
  • Loading branch information
Xanewok committed Aug 11, 2018
1 parent ea2dfca commit d04d407
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 0 deletions.
85 changes: 85 additions & 0 deletions pkg/nuclide-rust/lib/BuckIntegration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the license found in the LICENSE file in
* the root directory of this source tree.
*
* @flow
* @format
*/

import type {TaskInfo} from '../../nuclide-buck/lib/types';
import type {
AtomLanguageService,
LanguageService,
} from '../../nuclide-language-service';

import invariant from 'invariant';
import {getLogger} from 'log4js';
import fsPromise from 'nuclide-commons/fsPromise';
import nuclideUri from 'nuclide-commons/nuclideUri';
import {getRustInputs, getSaveAnalysisTargets, normalizeNameForBuckQuery} from './BuckUtils';

import * as BuckService from '../../nuclide-buck-rpc';

const logger = getLogger('nuclide-rust');

export async function updateRlsBuildForTask(
task: TaskInfo,
service: AtomLanguageService<LanguageService>,
) {
const buildTarget = normalizeNameForBuckQuery(task.buildTarget);

This comment has been minimized.

Copy link
@mostafaeweda

mostafaeweda Aug 13, 2018

Could you filter out non-rust tasks / build targets? (that'd save a couple of expensive unnecessary buck query calls for non-buck targets?

This comment has been minimized.

Copy link
@Xanewok

Xanewok Aug 16, 2018

Author Owner

Sure thing, I'll add the rule filter in the getRustInputs


const files = await getRustInputs(task.buckRoot, buildTarget);
// Probably not a Rust build target, ignore
if (files.length == 0)
return;
// We need only to pick a representative file to get a related lang service
const fileUri = task.buckRoot + '/' + files[0];
atom.notifications.addInfo(`fileUri: ${fileUri}`);

This comment has been minimized.

Copy link
@mostafaeweda

mostafaeweda Aug 13, 2018

remove?

This comment has been minimized.

Copy link
@Xanewok

Xanewok Aug 16, 2018

Author Owner

Right, just left it there since I was still to check the remote Nuclide workflow


const langService = await service.getLanguageServiceForUri(fileUri);
invariant(langService != null);

// Since `buck` execution is not trivial - the command may be overriden, needs
// to inherit the environment, passes internal FB USER to env etc. the RLS
// can't just invoke that.
// Instead, we build now, copy paths to resulting .json analysis artifacts to
// a temp file and just use `cat $TMPFILE` as a dummy build command.
const analysisTargets = await getSaveAnalysisTargets(task.buckRoot, buildTarget);
logger.debug(`analysisTargets: ${analysisTargets.join('\n')}`);
let artifacts: Array<string> = [];

const buildReport = await BuckService.build(task.buckRoot, analysisTargets);
if (buildReport.success === false) {
atom.notifications.addError("save-analysis build failed");
return;
}

Object.values(buildReport.results)
// TODO: https://buckbuild.com/command/build.html specifies that for
// FETCHED_FROM_CACHE we might not get an output file - can we force it
// somehow? Or we always locally produce a save-analysis .json file?
.forEach((targetReport: any) => artifacts.push(targetReport.output));

const tempfile = await fsPromise.tempfile();
await fsPromise.writeFile(tempfile, artifacts.join('\n'));

// TODO: Windows?
const buildCommand = `cat ${tempfile}`;

This comment has been minimized.

Copy link
@mostafaeweda

mostafaeweda Aug 13, 2018

Why pass a command instead of passing the artifacts directly?
Also, what's the lifecycle of that tempfile, when will it be deleted?

This comment has been minimized.

Copy link
@Xanewok

Xanewok Aug 16, 2018

Author Owner

So the idea was that the specified command in the RLS itself should perform the build on its own rather than RLS be fed the results. Since the execution is not as easy due to how Buck can be spawned and overriden, in this case this is why I hooked up on after the Buck build in Nuclide and am feeding the results directly.

Re tempfile I'm not sure, it should probably be spawned on a tmp dir as governed by OS (think /tmp/ in Linux) and managed by it. I'll be honest and just copied what was used in other places in Nuclide for things like that.

The file itself is required for a very brief amount of time, so any other temp file OS policy other than 'remove the temp file the instant it's created' should be relatively safe.


logger.debug(`Built SA artifacts: ${artifacts.join('\n')}`);
logger.debug(`buildCommand: ${buildCommand}`);

langService.sendLspNotification(fileUri, 'workspace/didChangeConfiguration',

This comment has been minimized.

Copy link
@Xanewok

Xanewok Aug 16, 2018

Author Owner

Changing the configuration retriggers the build immediately due to how RLS works, I'll add a comment.

{
settings: {
rust: {
unstable_features: true, // Required for build_command
build_on_save: true,
build_command: buildCommand, // TODO: Only in RLS branch: https://github.com/Xanewok/rls/tree/external-build
}
}
});
}
50 changes: 50 additions & 0 deletions pkg/nuclide-rust/lib/BuckUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the license found in the LICENSE file in
* the root directory of this source tree.
*
* @flow strict-local
* @format
*/

import type {TaskInfo} from '../../nuclide-buck/lib/types';

import * as BuckService from '../../nuclide-buck-rpc';

export function getRustInputs(buckRoot: string, buildTarget: BuildTarget): Promise<Array<string>> {
return BuckService.
query(buckRoot, `filter('.*\\.rs$', inputs('${buildTarget}'))`, []);
}

export function getSaveAnalysisTargets(buckRoot: string, buildTarget: BuildTarget): Promise<Array<string>> {
// Save-analysis build flavor is only supported by rust_{binary, library}
// kinds (so exclude prebuilt_rust_library kind)
const query: string = `kind('^rust_.*', deps(${buildTarget}))`;

return BuckService.query(buckRoot, query, []).then(deps =>
deps.map(dep => dep + '#save-analysis'),
);
}

export type BuildTarget = string;

// FIXME: Copied from nuclide-buck-rpc
// Buck query doesn't allow omitting // or adding # for flavors, this needs to be fixed in buck.
export function normalizeNameForBuckQuery(aliasOrTarget: string): BuildTarget {
let canonicalName = aliasOrTarget;
// Don't prepend // for aliases (aliases will not have colons or .)
if (
(canonicalName.indexOf(':') !== -1 || canonicalName.indexOf('.') !== -1) &&
canonicalName.indexOf('//') === -1
) {
canonicalName = '//' + canonicalName;
}
// Strip flavor string
const flavorIndex = canonicalName.indexOf('#');
if (flavorIndex !== -1) {
canonicalName = canonicalName.substr(0, flavorIndex);
}
return canonicalName;
}
5 changes: 5 additions & 0 deletions pkg/nuclide-rust/lib/RustLanguage.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ async function connectionToRustService(
useOriginalEnvironment: true,
additionalLogFilesRetentionPeriod: 5 * 60 * 1000, // 5 minutes
waitForDiagnostics: true,
initializationOptions: {
// Don't let RLS eagerly build (and fail crashing while finding a
// Cargo.toml if the project uses Buck) for now.
omitInitBuild: true,
},
},
);

Expand Down
4 changes: 4 additions & 0 deletions pkg/nuclide-rust/lib/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import createPackage from 'nuclide-commons-atom/createPackage';
import UniversalDisposable from 'nuclide-commons/UniversalDisposable';
import {createRustLanguageService} from './RustLanguage';

import {updateRlsBuildForTask} from './BuckIntegration';

class Activation {
_rustLanguageService: AtomLanguageService<LanguageService>;
_buckTaskRunnerService: ?BuckTaskRunnerService;
Expand All @@ -32,6 +34,8 @@ class Activation {
}

consumeBuckTaskRunner(service: BuckTaskRunnerService): IDisposable {
service.onDidCompleteTask(task => updateRlsBuildForTask(task, this._rustLanguageService));

this._buckTaskRunnerService = service;
return new UniversalDisposable(() => {
this._buckTaskRunnerService = null;
Expand Down

2 comments on commit d04d407

@mostafaeweda
Copy link

Choose a reason for hiding this comment

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

The current experience is non-optimal as users have to input & build buck targets from the toolbar & only then, chain extra buck build commands (without reporting progress), & finally, the users would get their language service working.

@Xanewok
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I agree that's not optimal right now.

From what I understand, the MultiLspLanguageService acts as a single service that spawns sub-services when it determines the appropriate project dir using a factory.
However, I couldn't find a callback for when that happens and I also can't specify the build command on initializationOptions since that's static information. Ideally I could run the equivalent of buck build (whichever targets correspond to `..` with #save-analysis) directly when initializing an LSP server or after it's spawned (preferably the latter) - this way the user wouldn't have to do anything else to get its index-based features for Rust projects.

Please sign in to comment.