[makefile] highlighting issues with variable definitions and shell commands

[xornet-sl]

• VSCode Version: 1.25.1
• OS Version: Linux Arch (updated)

Variables in braces

According to this document (GNU Makefile reference) there are two allowed types of variable definitions: $(VARIABLE) and ${VARIABLE}:

To substitute a variable’s value, write a dollar sign followed by the name of the variable in parentheses or braces: either ‘$(foo)’ or ‘${foo}’ is a valid reference to the variable foo.

Currently Makefile highlighting definition supports only parentheses but not braces.

Shell commands with $()

Look at:

all:
    some_shell_var=$$(sed -nre 's/some regex with (group)/\1/p')

sed command is parsed as makefile function, just like $() Even when we escape dollar sign with another dollar sign. So in this case highlighting totally brakes on closing parenthesis inside sed's regexp.

[aeschli]

@fadeevab FYI We're using your grammar from https://github.com/fadeevab/make.tmbundle (Issues are disabled, so I can't move it there)

[fadeevab]

@aeschli, Thanks for notification, I didn't see that. I need to check to enable issues. And I'll check current issue little bit later today.

[fadeevab]

@xornet-sl,

1. Yes, I can fix ${}.
2. Your example of shell is not a shell: $(sed -nre 's/some regex with (group)/\1/p') is a variable, right syntax is $(shell sed -rne 's/.......'). So in your case the line is colored as variable because $(sed ..) is a variable. Other parentheses are not expected inside a variable. If you change your example to $$(shell ...) you could see that colors are correct.

[xornet-sl]

@fadeevab No!!! Please, look closer what I wrote in topic. I use two dollar signs before sed command. It should be passed to shell just like $(sed) and shouldn't be parsed as Makefile variable. In this case $(sed ...) will be evaluated by shell, not make. This topic also described in GNU Makefile reference p. 5.1.2:

Variable and function references in recipes have identical syntax and semantics to references elsewhere in the makefile. They also have the same quoting rules: if you want a dollar sign to appear in your recipe, you must double it (‘$$’). For shells like the default shell, that use dollar signs to introduce variables, it’s important to keep clear in your mind whether the variable you want to reference is a make variable (use a single dollar sign) or a shell variable (use two dollar signs).

[fadeevab]

Hm, I've got your point. Actually, currently the parser is developed to colorize an expression even after $$, it's useful to see the expected contents of complex makefile variables after demangling. For your case, it seems reasonable to disable such colorization at all for $$ in a target's recipe. What do you think about such option? Need to think about how could it be implemented in a straightforward way...

[fadeevab]

@aeschli

Before, I tested TextMate xml syntax on https://github-lightshow.herokuapp.com/, but now it's down. Don't you know, is there any other method to check TextMate XML syntax? For instance, I could use any "TextMate XML to JSON converter" if there is any, then I would able to test final JSON (converted from xml) in VSCode. I find nothing, maybe you know any way to test. Thank you for any suggestion.

[aeschli]

@fadeevab VSCode also can load grammars in plist format. Just give the file the plist suffix...

[fadeevab]

@aeschli, great!

[fadeevab]

FYI

@aeschli, Putting *.plist into installation directory of VSCode doesn't work, and I don't know about to launch code from development directory, but I'm not able to set up VSCode development project right now.

So a simple way, that I found for myself to quickly test TextMate XML grammar:

Prerequisites:

1. git clone https://github.com/Microsoft/vscode.git
2. sudo apt-get install npm
3. (maybe some deps were installed previously via yarn before)

Then:

1. cd ./vscode/extensions/{lang}
2. put the grammar file (XML) into ./syntaxes/{lang}.tmLanguage.plist
3. npm run update-grammar
4. sudo cp ./syntaxes/{lang}.tmLanguage.json /usr/share/code/resources/app/extensions/{lang}/syntaxes/
5. code (F1->"Reload Window" if launched)

[aeschli]

You also have to change the path to the grammar in the extensions/{lang}/package.json file

[fadeevab]

The easiest way to do XML TextMate -> JSON:

1. npm install fast-plist
2. create plist-to-json.js (any filename), and fix filenames inside the code:

   
   // plist-to-json.js
   var parse = require('fast-plist').parse;
   var fs = require('fs');

var contents = fs.readFileSync("Makefile.plist", 'utf8'); var result = JSON.stringify(parse(contents), null, '\t'); fs.writeFileSync("make.tmLanguage.json", result);

3. `node plist-to-json.js`

[fadeevab]

@aeschli Anyway, the patch is in my repo. I would like to add tests into VSCode about makefile syntax, but in case of delay from my side, pull the patches from my repo. Thanks

[xornet-sl]

@fadeevab

Hm, I've got your point. Actually, currently the parser is developed to colorize an expression even after $$, it's useful to see the expected contents of complex makefile variables after demangling. For your case, it seems reasonable to disable such colorization at all for $$ in a target's recipe. What do you think about such option? Need to think about how could it be implemented in a straightforward way...

Yes, I think it should be a nice idea to allow users customize syntax highlighter behaviour.

[fadeevab]

@aeschli,

I'm not sure about the proper sequence now, but what do I have:

1. Solution of current issue is in my repo: https://github.com/fadeevab/make.tmbundle. Do you have to get updated grammar from the repo by scripts, or do you need my pull request with updated JSON from my vscode fork together with tests?
2. You can see my pull request containing only tests about new improvements: https://github.com/Microsoft/vscode/pull/55826. Shortly, those tests will fail until you get updated grammar from repo.

So, everything is here, just need to wrap it.

[aeschli]

Thanks for adding tests! I'll update the grammar, add your tests, and update the tests. These are the steps (if you want to provide a self-contained PR next time).

• cd extensions/make, npm run update-grammar
• apply changes to the test files
• compile vscode workspace and run tests ('yarn', 'yarn install,./scripts/test-integration.sh/bat`)
• the tests will modify the result files. Go through the changes and make sure they make sense.
• make a PR of all the changes.

[fadeevab]

@aeschli,

Got it, right. Thank you!