Skip to content

Commit 8209f88

Browse files
committed
Give targets that link torch_cpu_library torch's compile settings back
Linking the torch target does more than link. TorchConfig also gives it TORCH_INCLUDE_DIRS, which CMake treats as system headers because the target is imported, TORCH_CXX_FLAGS, and on recent torch a minimum C++ standard (C++20). torch_cpu_library, which our targets now link instead so that a CPU-only torch can load them, only passes on torch/include. So those targets changed. torch/csrc/api/include became a normal include directory, so warnings inside torch's headers are now reported as if they were our code. And a TORCH_CXX_FLAGS set at configure time no longer reached them. For example, compiling kernels/portable/cpu/op_allclose.cpp with -Wall -Wextra -Werror went from no errors to 25 errors, all inside torch's headers. Default builds were not affected. find_package_torch now copies torch's interface include directories, compile options and compile features to torch_cpu_library. It does this once, also when the project found torch before calling it. Every target that links torch_cpu_library, directly or through a PUBLIC link such as custom_ops_aot_lib, now gets these settings, and a new link site cannot forget them. With a CUDA build of PyTorch, torch also carries the CUDA include directory and definitions of torch_cuda_library. Those are not copied, since these targets do not use CUDA. Changing torch_cpu_library itself also affects any other target in the build that links it. When there is no torch_cpu_library, for example with a static libtorch or a parent project that defines its own torch target, nothing is copied. A new test configures a small project and checks that a target linking torch_cpu_library, directly or through a PUBLIC link, gets torch's C++ API include directory, TORCH_CXX_FLAGS and C++ standard. It checks both when find_package_torch finds torch and when the project found it first. When torch needs no -std= flag with the current compiler, the C++ standard check is reported as skipped. A second test checks that find_package_torch still configures when only a torch target exists. Both pass with CPU and CUDA builds of torch. The first fails without this change, and the second fails if the copy does not first check that torch_cpu_library exists.
1 parent e14d655 commit 8209f88

2 files changed

Lines changed: 173 additions & 0 deletions

File tree

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
# All rights reserved.
3+
#
4+
# This source code is licensed under the BSD-style license found in the
5+
# LICENSE file in the root directory of this source tree.
6+
7+
import importlib.util
8+
import json
9+
import shlex
10+
import shutil
11+
import subprocess
12+
import sys
13+
import tempfile
14+
import unittest
15+
from pathlib import Path
16+
17+
ROOT = Path(__file__).resolve().parents[3]
18+
19+
20+
def _compile_flags(entry: dict) -> set:
21+
"""The flags of one compile command, without the compiler, source and outputs."""
22+
flags = set()
23+
arguments = iter(shlex.split(entry["command"])[1:])
24+
for argument in arguments:
25+
if argument in ("-o", "-c", "-MT", "-MF"):
26+
next(arguments)
27+
elif argument == "-isystem":
28+
flags.add(f"-isystem {next(arguments)}")
29+
elif argument != "-MD":
30+
flags.add(argument)
31+
return flags
32+
33+
34+
def _copied_from_torch(flag: str) -> bool:
35+
"""Whether a flag comes from what find_package_torch copies from torch.
36+
37+
A CUDA build of torch also gives torch the CUDA toolkit include dir and torch_cuda's
38+
definitions, which torch_cpu_library correctly does not carry.
39+
"""
40+
return (
41+
flag == "-DET_TORCH_CXX_FLAGS_PROBE"
42+
or flag.startswith("-std=")
43+
or (flag.startswith("-isystem ") and flag.endswith("torch/csrc/api/include"))
44+
)
45+
46+
47+
@unittest.skipIf(sys.platform == "win32", "Visual Studio writes no compile commands")
48+
class TestTorchCpuLibraryCMake(unittest.TestCase):
49+
def test_torch_cpu_library_gets_torch_settings(self):
50+
cmake = shutil.which("cmake")
51+
if cmake is None or importlib.util.find_spec("torch") is None:
52+
self.skipTest("CMake and torch are required")
53+
54+
found_first = (
55+
"add_torch_to_cmake_prefix_path()\nfind_package(Torch CONFIG REQUIRED)"
56+
)
57+
for find_torch in ("", found_first):
58+
with self.subTest(found_first=bool(find_torch)):
59+
self._check(cmake, find_torch)
60+
61+
def _check(self, cmake: str, find_torch: str) -> None:
62+
with tempfile.TemporaryDirectory() as directory:
63+
source = Path(directory)
64+
build = source / "build"
65+
(source / "probe.cpp").write_text("int probe() { return 0; }\n")
66+
(source / "CMakeLists.txt").write_text(
67+
f"""cmake_minimum_required(VERSION 3.24)
68+
project(TorchCpuLibraryProbe LANGUAGES CXX)
69+
include("${{EXECUTORCH_ROOT}}/tools/cmake/Utils.cmake")
70+
{find_torch}
71+
find_package_torch()
72+
add_library(links_torch OBJECT probe.cpp)
73+
target_link_libraries(links_torch PRIVATE torch)
74+
add_library(links_torch_cpu_library OBJECT probe.cpp)
75+
target_link_libraries(links_torch_cpu_library PRIVATE torch_cpu_library)
76+
add_library(links_it_publicly STATIC probe.cpp)
77+
target_link_libraries(links_it_publicly PUBLIC torch_cpu_library)
78+
add_library(consumer OBJECT probe.cpp)
79+
target_link_libraries(consumer PRIVATE links_it_publicly)
80+
"""
81+
)
82+
result = subprocess.run(
83+
[
84+
cmake,
85+
"-S",
86+
str(source),
87+
"-B",
88+
str(build),
89+
f"-DEXECUTORCH_ROOT={ROOT}",
90+
f"-DPYTHON_EXECUTABLE={sys.executable}",
91+
"-DCMAKE_EXPORT_COMPILE_COMMANDS=ON",
92+
# Recent torch sets no TORCH_CXX_FLAGS, so set one to check it travels too.
93+
"-DTORCH_CXX_FLAGS=-DET_TORCH_CXX_FLAGS_PROBE",
94+
],
95+
capture_output=True,
96+
text=True,
97+
)
98+
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
99+
100+
flags = {}
101+
for entry in json.loads((build / "compile_commands.json").read_text()):
102+
target = Path(entry["output"]).parent.name.removesuffix(".dir")
103+
flags[target] = _compile_flags(entry)
104+
expected = {
105+
flag for flag in flags["links_torch"] if _copied_from_torch(flag)
106+
}
107+
self.assertIn("-DET_TORCH_CXX_FLAGS_PROBE", expected)
108+
self.assertTrue(any(flag.startswith("-isystem ") for flag in expected))
109+
for target in ("links_torch_cpu_library", "links_it_publicly", "consumer"):
110+
self.assertEqual(expected - flags[target], set(), target)
111+
with self.subTest("C++ standard"):
112+
if not any(flag.startswith("-std=") for flag in expected):
113+
self.skipTest("this compiler needs no -std= flag for torch")
114+
115+
def test_find_package_torch_without_torch_cpu_library(self):
116+
"""A parent project or a static libtorch can define torch alone."""
117+
cmake = shutil.which("cmake")
118+
if cmake is None:
119+
self.skipTest("CMake is required")
120+
with tempfile.TemporaryDirectory() as directory:
121+
source = Path(directory)
122+
(source / "CMakeLists.txt").write_text(
123+
"""cmake_minimum_required(VERSION 3.24)
124+
project(TorchWithoutTorchCpuLibrary LANGUAGES NONE)
125+
include("${EXECUTORCH_ROOT}/tools/cmake/Utils.cmake")
126+
add_library(torch INTERFACE)
127+
find_package_torch()
128+
"""
129+
)
130+
result = subprocess.run(
131+
[
132+
cmake,
133+
"-S",
134+
str(source),
135+
"-B",
136+
str(source / "build"),
137+
f"-DEXECUTORCH_ROOT={ROOT}",
138+
],
139+
capture_output=True,
140+
text=True,
141+
)
142+
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
143+
144+
145+
if __name__ == "__main__":
146+
unittest.main()

‎tools/cmake/Utils.cmake‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,33 @@ macro(find_package_torch)
278278
add_torch_to_cmake_prefix_path()
279279
find_package(Torch CONFIG REQUIRED)
280280
endif()
281+
# TorchConfig gives its include dirs, compile flags and C++ standard to torch
282+
# only. Our targets link torch_cpu_library instead, so copy them there once,
283+
# also when the project found torch before calling this. This affects every
284+
# target in the build that links torch_cpu_library.
285+
if(TARGET torch_cpu_library)
286+
get_target_property(
287+
_torch_settings_copied torch_cpu_library EXECUTORCH_TORCH_SETTINGS_COPIED
288+
)
289+
if(NOT _torch_settings_copied)
290+
foreach(_torch_property
291+
INTERFACE_INCLUDE_DIRECTORIES INTERFACE_COMPILE_OPTIONS
292+
INTERFACE_COMPILE_FEATURES
293+
)
294+
get_target_property(_torch_value torch ${_torch_property})
295+
if(_torch_value)
296+
set_property(
297+
TARGET torch_cpu_library
298+
APPEND
299+
PROPERTY ${_torch_property} ${_torch_value}
300+
)
301+
endif()
302+
endforeach()
303+
set_property(
304+
TARGET torch_cpu_library PROPERTY EXECUTORCH_TORCH_SETTINGS_COPIED TRUE
305+
)
306+
endif()
307+
endif()
281308
endmacro()
282309

283310
# Modify ${targetName}'s INTERFACE_INCLUDE_DIRECTORIES by wrapping each entry in

0 commit comments

Comments
 (0)