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

support linux and osx specific command properties #5579

Merged
merged 1 commit into from
Jul 2, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Jun 25, 2019

Signed-off-by: elaihau [email protected]

@elaihau
Copy link
Contributor Author

elaihau commented Jun 25, 2019

@RomanNikitenko @azatsarynnyy I went through the source code of Theia and VS Code, looks like neither have OS-specific command properties support in the plugin-ext (extensionHost in VS Code). So I guess, plugin developers are supposed to deal with the operating system specific stuffs in the plugin code. Am I right ?

@elaihau elaihau force-pushed the task_osx_linux branch 2 times, most recently from 0df348e to f175c54 Compare June 25, 2019 19:06
@azatsarynnyy
Copy link
Member

@elaihau OS specific properties can be defined for the process/shell task types only. Which are kind of internal types for Theia/VS Code.
That means the mechanism of overriding the properties (command, args, options) doesn't work for the tasks contributed by the extensions.
So an extension developer doesn't care about it.

@azatsarynnyy
Copy link
Member

I've tested the PR on Linux - it works well.

@elaihau
Copy link
Contributor Author

elaihau commented Jun 27, 2019

could we test on mac together @marechal-p when you have time ?

systemSpecificCommand = this.getSystemSpecificCommand(taskConfig, 'osx');
} else { // on linux
if (taskConfig.linux !== undefined) { // linux-specific options, if available, take precedence
systemSpecificCommand = this.getSystemSpecificCommand(taskConfig, 'linux');
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I don't have a chance to test it on Windows/MacOS.
Let's assume I have the following task which I'm going run on Windows/MacOS:

		{
			"label": "Run Dev",
			"type": "shell",
			"command": ".\\scripts\\code.bat",
			"linux": {
				"command": "./scripts/code.sh"
			}
		}

If I understand it right, Theia would try to execute the command from linux.command property. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you run that task on Windows/OSX, ".\scripts\code.bat" should be run. linux.command is only for execution on linux.
please correct me if my code is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, i see what you mean - there is a bug with the if - else logic here. let me do more testing and fix it.

@elaihau elaihau force-pushed the task_osx_linux branch 2 times, most recently from 72579be to feac4ad Compare June 27, 2019 17:19
- as per https://github.com/microsoft/vscode/blob/1.35.1/src/vs/workbench/contrib/tasks/common/taskConfiguration.ts#L255, vsCode supports having separated command properties for Windows, OSX, and Linux. This change adds the same support to Theia.
- changed the `command` property in CommandProperties interface from mandatory to optional.
- part of eclipse-theia#5516

Signed-off-by: elaihau <[email protected]>
@elaihau
Copy link
Contributor Author

elaihau commented Jun 27, 2019

Tested on mac with @marechal-p . Also added mac-specific unit tests as per his comments.

@elaihau elaihau merged commit 65ffea5 into eclipse-theia:master Jul 2, 2019
@elaihau elaihau deleted the task_osx_linux branch July 2, 2019 13:16
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.

2 participants