(makefile) Syntax highlighting of Makefiles #3878

TriMoon · 2023-10-03T18:25:37Z

Describe the issue
Syntax highlighting of Makefiles is bugged.

Which language seems to have the issue?
lang-makefile

Are you using highlight or highlightAuto?
no idea what Stackexchange uses...

Sample Code to Reproduce

demo

#	SPDX-License-Identifier: CC-BY-NC-SA-4.0
#
SHELL = /usr/bin/bash
.SHELLFLAGS = -ec
.ONESHELL:
.SILENT:

UTILS			:=
UTILS			+= setfacl
UTILS			+= chown
UTILS			+= chmod
# UTILS			+= nonexisting

$(info ### START: Utils check)
# Define macro wrt shell.
ifeq (bash,$(findstring bash,$(SHELL)))
toUpper	= $(shell string="$(strip $1)"; printf "%s" $${string^^})
else
# Insipred by: https://stackoverflow.com/a/37660916/22510042
toUpper = $(shell echo "$(strip $1)" | sed 's/.*/\U&/')
endif
# Inspired by: https://stackoverflow.com/a/37197276/22510042
$(foreach util,shell $(UTILS),\
	$(if $(filter-out shell,$(util)),\
		$(if $(shell command -v $(util) 2> /dev/null),\
			$(eval \
				$(shell \
					printf "%s\t:=\t%s\n" \
						$(call toUpper,$(util) ) \
						$(shell command -v $(util) 2> /dev/null)\
				)\
			)\
			,$(error No '$(util)' in PATH)\
		)\
	)\
	$(info $(call toUpper,$(util))	= $($(call toUpper,$(util))))\
)
# There is a \t char before the = in the info line above for alignment...
$(info ### END: Utils check)
$(info ) # Empty line
# $(error END: Forced)

Expected behavior

Something similar to how kate renders it, although that has some errors also... (see the orange lines)

Additional context

See the Meta discussion: https://meta.stackexchange.com/q/393515/1288919

The text was updated successfully, but these errors were encountered:

joshgoebel · 2023-10-08T02:19:10Z

Well the "all green" after a certain point is likely a bug in how we handle strings, but if that sample is any indicator it looks like our handling of makefiles is pretty poor. Would definitely be open to someone stepping up and improving things here. The grammar is currently pretty simple and should be easier to build on than some.

aneesh98 · 2023-10-14T16:06:31Z

Hi Would like to work on this issue

joshgoebel · 2023-10-15T04:44:47Z

Go for it. :)

aneesh98 · 2023-10-21T18:37:40Z

Hi I am still working on the fix for this issue, and I noticed that the "dockerfile.js" present "src/languages" is being interpreted as a Dockerfile in VSCode (which I am using for development). Leading to incorrect syntax highlighting.
Check below screenshot for reference.

I observed this also happens if I make files with any other extension (e.g. .py files for python).
While this has got nothing to do with the highlight.js. I just wanted to open an issue in VSCode, and cite this file from the project as example where the issue occurs in VSCode. Hope that's fine?

joshgoebel · 2023-10-21T21:34:04Z

All our code is open source, share it wherever... But I think one docker convention is that Dockerfile.* can be a dockerfile... so that might technically be correct in some sense, despite also being very wrong. Curious what the VS Code people will say.

aneesh98 · 2023-10-22T09:03:02Z

HI @joshgoebel .
I have made the following change,
Currently the highlighting issue starts from the definition of toUpper macro within the ifeq statement:

ifeq (bash,$(findstring bash,$(SHELL)))
toUpper	= $(shell string="$(strip $1)"; printf "%s" $${string^^})

I believe this is happening because the first string mentioned in the function call '"$(strip $1)"' is not being identified correctly and I think the cause is 'FUNC' variable in makefile.js doesn't currently include QUOTE_STRING

highlight.js/src/languages/makefile.js

Lines 32 to 42 in 17494c8

    
           const FUNC = { 
        
             className: 'variable', 
        
             begin: /\$\([\w-]+\s/, 
        
             end: /\)/, 
        
             keywords: { built_in: 
        
                 'subst patsubst strip findstring filter filter-out sort ' 
        
                 + 'word wordlist firstword lastword dir notdir suffix basename ' 
        
                 + 'addsuffix addprefix join wildcard realpath abspath error warning ' 
        
                 + 'shell origin flavor foreach if or and call eval file value' }, 
        
             contains: [ VARIABLE ] 
        
           };

If I change the same to include QUOTE_STRING as done in my PR

https://github.com/aneesh98/highlight.js/blob/904ddd1295e48c1097795144b8bf21a533a8c6cf/src/languages/makefile.js#L32-L45

This is the obtained result.

This does solve the initial formatting issue.
But I observe few things here.
The string "$(strip $1)" is a function call is not being highlighted as per function call rules of Makefile but is being highlighted as a plain string, as opposed to the screenshot posted under "expected behaviour" for this issue.

I am trying to fix this, but unable to understand on how I can proceed 😅 . Can you please help?
Another few things I observe is function "filter-out" has two formatting one before the hyphen and one after the hyphen could this be a RegExp issue?.

Kindly help. Thanks. 😄

joshgoebel · 2023-10-22T20:28:12Z

Keywords match in the order defined, so reverse the order of filter and filter-out.

If we're going to start including more things in $() then it's no longer only variables we're dealing with so we should remove that top-level scope... perhaps add another rule to detect only SIMPLE variables like $(name_here)...

I'm not sure exactly what the context of the code inside the `$() is though vs that outside.

TriMoon · 2023-10-23T01:42:08Z

@aneesh98

But I observe few things here.
The string "$(strip $1)" is a function call is not being highlighted as per function call rules of Makefile but is being highlighted as a plain string

Maybe that's because of:

highlight.js/src/languages/makefile.js

Lines 11 to 12 in 17494c8

    
           const VARIABLE = { 
        
             className: 'variable',

highlight.js/src/languages/makefile.js

Lines 32 to 33 in 17494c8

    
           const FUNC = { 
        
             className: 'variable',

I am trying to fix this, but unable to understand on how I can proceed 😅 . Can you please help?

hence the value of className needs to be different? 🤔 😉
https://github.com/highlightjs/highlight.js/blob/main/docs/css-classes-reference.rst

joshgoebel · 2023-10-23T01:55:25Z

Correct or blank, but I'm not familiar enough with Makefile to know what that content is...

TriMoon · 2023-10-23T02:22:30Z

"$(strip $1)" = The result is a string.
$(strip $1) = Is a function call.
strip = the function name.
$1 = the argument to the function.
Multiple arguments are separated by a comma if that function defines that...
In the case of the shell function they are separated by space like in a normal invocation of a (bash/sh) shell function.

aneesh98 · 2023-10-23T17:56:26Z

Keywords match in the order defined, so reverse the order of filter and filter-out.

If we're going to start including more things in $() then it's no longer only variables we're dealing with so we should remove that top-level scope... perhaps add another rule to detect only SIMPLE variables like $(name_here)...

I'm not sure exactly what the context of the code inside the `$() is though vs that outside.

I understood for filter and filter-out, thanks for the explanation. I will make the change.
Little confused about what you said after that 😅

Is it regarding having function call and multiple arguments in $() like $(foreach util,shell $(UTILS)

What do you mean by top-level scope here?

If we're going to start including more things in $() then it's no longer only variables we're dealing with so we should remove that top-level scope

aneesh98 · 2023-10-23T17:57:21Z

@aneesh98

But I observe few things here.
The string "$(strip $1)" is a function call is not being highlighted as per function call rules of Makefile but is being highlighted as a plain string

Maybe that's because of:

highlight.js/src/languages/makefile.js

Lines 11 to 12 in 17494c8

const VARIABLE = {

className: 'variable',

highlight.js/src/languages/makefile.js

Lines 32 to 33 in 17494c8

const FUNC = {

className: 'variable',

I am trying to fix this, but unable to understand on how I can proceed 😅 . Can you please help?

hence the value of className needs to be different? 🤔 😉 https://github.com/highlightjs/highlight.js/blob/main/docs/css-classes-reference.rst

Ahh yess. Guess the FUNC variable needs a class name change, will look into this. Thanks

TriMoon · 2023-11-14T13:11:57Z

FYI: The current changes in #3898 are not adequate to fix the problem in the OP.
So if others want to help or take-over the work of @aneesh98 feel free 😉

Note

To assist you guys in improving the highlighting of makefiles:
The latest version of the main functionality of this Makefile is available in one-of-my repo's at GitLab. 😉🤝
(Who knows maybe it will serve you also on occasions)

TriMoon added bug help welcome Could use help from community language labels Oct 3, 2023

joshgoebel added the good first issue Should be easier for first time contributors label Oct 8, 2023

aneesh98 linked a pull request Oct 22, 2023 that will close this issue

Makefile String Fix #3898

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(makefile) Syntax highlighting of Makefiles #3878

(makefile) Syntax highlighting of Makefiles #3878

TriMoon commented Oct 3, 2023

joshgoebel commented Oct 8, 2023

aneesh98 commented Oct 14, 2023

joshgoebel commented Oct 15, 2023

aneesh98 commented Oct 21, 2023 •

edited

joshgoebel commented Oct 21, 2023

aneesh98 commented Oct 22, 2023 •

edited

joshgoebel commented Oct 22, 2023

TriMoon commented Oct 23, 2023 •

edited

joshgoebel commented Oct 23, 2023

TriMoon commented Oct 23, 2023 •

edited

aneesh98 commented Oct 23, 2023

aneesh98 commented Oct 23, 2023

TriMoon commented Nov 14, 2023 •

edited

(makefile) Syntax highlighting of Makefiles #3878

(makefile) Syntax highlighting of Makefiles #3878

Comments

TriMoon commented Oct 3, 2023

joshgoebel commented Oct 8, 2023

aneesh98 commented Oct 14, 2023

joshgoebel commented Oct 15, 2023

aneesh98 commented Oct 21, 2023 • edited

joshgoebel commented Oct 21, 2023

aneesh98 commented Oct 22, 2023 • edited

joshgoebel commented Oct 22, 2023

TriMoon commented Oct 23, 2023 • edited

joshgoebel commented Oct 23, 2023

TriMoon commented Oct 23, 2023 • edited

aneesh98 commented Oct 23, 2023

aneesh98 commented Oct 23, 2023

TriMoon commented Nov 14, 2023 • edited

aneesh98 commented Oct 21, 2023 •

edited

aneesh98 commented Oct 22, 2023 •

edited

TriMoon commented Oct 23, 2023 •

edited

TriMoon commented Oct 23, 2023 •

edited

TriMoon commented Nov 14, 2023 •

edited