Skip to content

Fix Ninja support #24584

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 17, 2025
Merged

Fix Ninja support #24584

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions tools/system_libs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import shutil
import textwrap
import shlex
import subprocess
from enum import IntEnum, auto
from glob import iglob
from typing import List, Optional
Expand Down Expand Up @@ -183,13 +184,15 @@ def create_ninja_file(input_files, filename, libname, cflags, asflags=None, cust
if asflags is None:
asflags = []

join_cmd = shlex.join if os.name == 'posix' else subprocess.list2cmdline
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just use subprocess.list2cmdline in all cases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see its a windows-specific function. I guess this will have to do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should re-add shlex_join to utils.py and use that instead of shlex.join everywhere. Effectivly reverting #24111 but with a cleaner shlex_join impl?

Copy link
Collaborator Author

@RReverser RReverser Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe, but not sure.

As I said in #24393 (comment), it does look like all other usages are for debug purposes only so syntax doesn't matter as much, and for command invocations we normally use subprocess module which does the right thing via native APIs automatically, so Ninja usecase - where we have to stringify a command with a valid OS-specific syntax - seems somewhat special in that regard.

If nothing else pops up, I wouldn't bother generalising it for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK sgtm


out = f'''\
# Automatically generated by tools/system_libs.py. DO NOT EDIT

ninja_required_version = 1.5

ASFLAGS = {shlex.join(asflags)}
CFLAGS = {shlex.join(cflags)}
ASFLAGS = {join_cmd(asflags)}
CFLAGS = {join_cmd(cflags)}
EMCC = {shared.EMCC}
EMXX = {shared.EMXX}
EMAR = {shared.EMAR}
Expand Down Expand Up @@ -267,7 +270,7 @@ def create_ninja_file(input_files, filename, libname, cflags, asflags=None, cust
if customize_build_flags:
custom_flags = customize_build_flags(flags, src)
if custom_flags != flags:
out += f' CFLAGS = {shlex.join(custom_flags)}'
out += f' CFLAGS = {join_cmd(custom_flags)}'
out += '\n'

objects = sorted(objects, key=objectfile_sort_key)
Expand Down