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

Shell expansion is not supported in compile_commands.json #670

Closed
Lassebq opened this issue Nov 8, 2024 · 12 comments · Fixed by #679
Closed

Shell expansion is not supported in compile_commands.json #670

Lassebq opened this issue Nov 8, 2024 · 12 comments · Fixed by #679
Assignees
Labels
enhancement New feature or request Feature: accuracy Able to parse and understand more scenarios Feature: Parsers The area of parsing the dry-run or build log
Milestone

Comments

@Lassebq
Copy link

Lassebq commented Nov 8, 2024

As per https://clang.llvm.org/docs/JSONCompilationDatabase.html neither "command" nor "arguments" key support shell expansion but makefile tools extension generates backticked shell commands regardless.

Example input makefile:

SRC = \
	src/main.c \
	build/main_ui.c

# OBJ = ${SRC:.c=.o}

BINNAME = bootroot

LIBS = \
	-lefivar

LIBS += `pkg-config --libs gtk4` -ladwaita-1

CFLAGS += `pkg-config --cflags gtk4`

all:
	mkdir -p build
	glib-compile-resources --sourcedir src/ui --generate-source --target build/main_ui.c src/ui/app_resources_ui.gresource.xml
	${CC} ${CFLAGS} -g -o ${BINNAME} ${SRC} ${LIBS}

Output compile database:

[
    {
        "command": "cc `pkg-config --cflags gtk4` -g -o bootroot src/main.c build/main_ui.c -lefivar `pkg-config --libs gtk4` -ladwaita-1",
        "directory": "/media/Stuff/git/bootroot",
        "file": "/media/Stuff/git/bootroot/src/main.c"
    },
    {
        "command": "cc `pkg-config --cflags gtk4` -g -o bootroot src/main.c build/main_ui.c -lefivar `pkg-config --libs gtk4` -ladwaita-1",
        "directory": "/media/Stuff/git/bootroot",
        "file": "/media/Stuff/git/bootroot/build/main_ui.c"
    }
]
@github-actions github-actions bot added the triage label Nov 8, 2024
@Yingzi1234
Copy link
Collaborator

@Lassebq We can't reproduce your question Based on the information you provided, can you answer the following question? It would be very helpful for us to investigate this question. Thank you very much!

  1. Could you provide how you generated compile_commands.json? Was it through bear or some other tool? What are the commands used?
  2. Could you provide the actual output of your pkg-config --cflags gtk4 and pkg-config --libs gtk4? This will help us understand the specific compiler flags and library paths!
  3. Which operating system (e.g. Ubuntu, macOS, etc.) and version are you using? This helps to know if it's related to a specific environment or toolchain
  4. Could you please provide your reproduction project?

@Yingzi1234 Yingzi1234 added more info needed More info is needed from the community for us to properly triage and investigate and removed triage labels Nov 13, 2024
@gcampbell-msft
Copy link
Collaborator

@Lassebq Is this essentially a feature request for us to recognize the command within your Makefile, execute it, and then replace the content when we drop it in your compile_commands.json?

Is the current functionality not working for you? (i.e. Intellisense isn't working)

@Lassebq
Copy link
Author

Lassebq commented Nov 13, 2024

@Yingzi1234

  1. It's generated by the extension with "makefile.compileCommandsPath" json key set to "compile_commands.json"
$ pkg-config --cflags --libs gtk4
-I/usr/include/gtk-4.0 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/graphene-1.0 -I/usr/lib/graphene-1.0/include -mfpmath=sse -msse -msse2 -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/fribidi -I/usr/include/sysprof-6 -pthread -I/usr/include/libpng16 -I/usr/include/pixman-1 -lgtk-4 -lharfbuzz -lpangocairo-1.0 -lpango-1.0 -lgdk_pixbuf-2.0 -lcairo-gobject -lcairo -lvulkan -lgraphene-1.0 -lgio-2.0 -lglib-2.0 -lgobject-2.0
  1. Alpine Linux, Arch Linux
  2. I've provided an example makefile in the issue. Content of source files doesn't matter.

@Lassebq
Copy link
Author

Lassebq commented Nov 13, 2024

@gcampbell-msft
This is not a feature request. The extension already makes compile_commands.json if you specify it's path via "makefile.compileCommandsPath". However, it's not valid. It doesn't follow the specification linked above and therefore clangd does not work properly. Shell expansion must be evaluated before being stored to the json.
Intellisense does not work.

@gcampbell-msft gcampbell-msft added bug Something isn't working enhancement New feature or request labels Nov 13, 2024
@gcampbell-msft gcampbell-msft added this to the On Deck milestone Nov 13, 2024
@gcampbell-msft gcampbell-msft added Feature: accuracy Able to parse and understand more scenarios Feature: Parsers The area of parsing the dry-run or build log and removed more info needed More info is needed from the community for us to properly triage and investigate labels Nov 13, 2024
@gcampbell-msft gcampbell-msft removed the bug Something isn't working label Dec 4, 2024
@gcampbell-msft gcampbell-msft modified the milestones: On Deck, 0.12.0 Dec 4, 2024
@gcampbell-msft gcampbell-msft self-assigned this Dec 11, 2024
@gcampbell-msft
Copy link
Collaborator

@Lassebq Could you please test out the vsix that is in the description of this draft PR? #679

You'll need to download the .zip file, modify the extension to be .vsix, and then install it manually in VS Code. Thanks!

@gcampbell-msft
Copy link
Collaborator

@Lassebq After a bunch of work to fix the test cases and test that it's working as expected, I think it's more ready than before, could you test this against your repro? I've tested locally and I believe it to be working. Again the vsix is in the description of the PR here: #679

@Lassebq
Copy link
Author

Lassebq commented Dec 17, 2024

Works as expected with "makefile.safeCommands" set.

@Lassebq
Copy link
Author

Lassebq commented Dec 17, 2024

But if you don't set it then the output is invalid. At least warn user about backtick commands being used in makefile. I was pulling my hair out trying to figure out what's wrong.

@gcampbell-msft
Copy link
Collaborator

gcampbell-msft commented Dec 17, 2024

@Lassebq The output of what is invalid? I would expect that the output is the same as it was before this support was added, we simply don't replace the command invocation.

We log into the output channel the message `"Commands must be marked as safe before they will be executed in the shell.". Is there more of a notification that you are expecting?

Thanks for testing.

@Lassebq
Copy link
Author

Lassebq commented Dec 17, 2024

I thought it was clear what's input and what's output in this context
Makefile - input
compile_command.json - output
Yes, logs aren't enough to make people aware of that right away. Usually people only read logs when they start troubleshooting the issue. You can avoid that altogether by making a notification.

let me quote this, regarding why the "output" (compile_commands.json) is invalid if you don't evaluate backtick commands:

command: The compile command as a single shell-escaped string. Arguments may be shell quoted and escaped following platform conventions, with ‘"’ and ‘\’ being the only special characters. Shell expansion is not supported.

source: https://clang.llvm.org/docs/JSONCompilationDatabase.html

@gcampbell-msft
Copy link
Collaborator

@Lassebq Okay, understood, so it wasn't that the output was worse than before, but it's still "invalid" in the llvm docs context. Got it, that makes sense.

As for the notification, we can add an error warning so that people aren't surprised by this and it'll be more obvious. Thanks for the feedback.

@gcampbell-msft
Copy link
Collaborator

@Lassebq Keeping you updated, I've pushed a commit that adds a vscode notification rather than only a message in the Makefile Tools output pane.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Feature: accuracy Able to parse and understand more scenarios Feature: Parsers The area of parsing the dry-run or build log
Projects
None yet
3 participants