[libc] Add some integration features into newhdrgen #114272

frobtech · 2024-10-30T17:18:45Z

This adds some command-line switches to the newhdrgen script
that facilitate more generalized build system integration.
These enable the use of the script in Fuchsia's GN build.

The --write-if-changed switch avoids touching the .h output file
if it's unchanged, which is beneficial to incremental rebuilds.

The --depfile switch writes out a make/ninja-compatible depfile
describing the inputs and outputs. This is crucial for build
system integration when using the following new feature.

When the new --libc-dir switch is given, the YAML file argument
is optional. Instead, the --h_def_file input file path must have
the --libc-dir path as a prefix and the yaml file is found in its
corresponding place in the libc source tree.

This adds some command-line switches to the newhdrgen script that facilitate more generalized build system integration. These enable the use of the script in Fuchsia's GN build. The --write-if-changed switch avoids touching the .h output file if it's unchanged, which is beneficial to incremental rebuilds. The --depfile switch writes out a make/ninja-compatible depfile describing the inputs and outputs. This is crucial for build system integration when using the following new feature. When the new --libc-dir switch is given, the YAML file argument is optional. Instead, the --h_def_file input file path must have the --libc-dir path as a prefix and the yaml file is found in its corresponding place in the libc source tree.

github-actions · 2024-10-30T17:22:32Z

✅ With the latest revision this PR passed the Python code formatter.

llvmbot · 2024-10-30T19:43:31Z

@llvm/pr-subscribers-libc

Author: Roland McGrath (frobtech)

Changes

This adds some command-line switches to the newhdrgen script
that facilitate more generalized build system integration.
These enable the use of the script in Fuchsia's GN build.

The --write-if-changed switch avoids touching the .h output file
if it's unchanged, which is beneficial to incremental rebuilds.

The --depfile switch writes out a make/ninja-compatible depfile
describing the inputs and outputs. This is crucial for build
system integration when using the following new feature.

When the new --libc-dir switch is given, the YAML file argument
is optional. Instead, the --h_def_file input file path must have
the --libc-dir path as a prefix and the yaml file is found in its
corresponding place in the libc source tree.

Full diff: https://github.com/llvm/llvm-project/pull/114272.diff

1 Files Affected:

(modified) libc/newhdrgen/yaml_to_classes.py (+64-11)

diff --git a/libc/newhdrgen/yaml_to_classes.py b/libc/newhdrgen/yaml_to_classes.py
index a295058f7dc821..460bc4a87452c3 100644
--- a/libc/newhdrgen/yaml_to_classes.py
+++ b/libc/newhdrgen/yaml_to_classes.py
@@ -219,15 +219,36 @@ def increase_indent(self, flow=False, indentless=False):
 def main():
     parser = argparse.ArgumentParser(description="Generate header files from YAML")
     parser.add_argument(
-        "yaml_file", help="Path to the YAML file containing header specification"
+        "yaml_file",
+        help="Path to the YAML file containing header specification",
+        type=Path,
+        nargs="?",
     )
     parser.add_argument(
         "--output_dir",
         help="Directory to output the generated header file",
+        type=Path,
     )
     parser.add_argument(
         "--h_def_file",
         help="Path to the .h.def template file (required if not using --export_decls)",
+        type=Path,
+    )
+    parser.add_argument(
+        "--libc-dir",
+        help="Path to llvm-libc source tree",
+        type=Path,
+        default=Path(__file__).parents[1],
+    )
+    parser.add_argument(
+        "--depfile",
+        help="Path to write a depfile",
+        type=argparse.FileType("w"),
+    )
+    parser.add_argument(
+        "--write-if-changed",
+        help="Don't touch the output .h file if it's unchanged",
+        action="store_true",
     )
     parser.add_argument(
         "--add_function",
@@ -252,30 +273,62 @@ def main():
     )
     args = parser.parse_args()
 
+    def write_to_file(path, contents):
+        if (
+            not args.write_if_changed
+            or not path.exists()
+            or path.read_text() != contents
+        ):
+            path.write_text(contents)
+
+    yaml_file = args.yaml_file
+    if args.h_def_file and not yaml_file:
+        libc_include_dir = args.libc_dir / "include"
+        libc_yaml_dir = args.libc_dir / "newhdrgen" / "yaml"
+        yaml_file = libc_yaml_dir / args.h_def_file.with_suffix("").with_suffix(
+            ".yaml"
+        ).relative_to(libc_include_dir)
+
+    if args.output_dir:
+        output_file_path = args.output_dir
+        if output_file_path.is_dir():
+            if not args.yaml_file:
+                libc_yaml_dir = args.libc_dir / "newhdrgen" / "yaml"
+                output_file_path /= args.yaml_file.relative_to(
+                    libc_yaml_dir
+                ).with_suffix(".h")
+            else:
+                output_file_path /= f"{Path(args.yaml_file).stem}.h"
+            output_file_path.parent.mkdir(parents=True, exist_ok=True)
+    else:
+        output_file_path = args.yaml_file.with_suffix(".h")
+
+    if args.depfile and args.h_def_file:
+        args.depfile.write(f"{output_file_path}: {args.h_def_file} {args.yaml_file}\n")
+        args.depfile.close()
+
     if args.add_function:
         add_function_to_yaml(yaml_file, args.add_function)
 
     header_class = GpuHeader if args.export_decls else HeaderFile
     header = load_yaml_file(args.yaml_file, header_class, args.entry_points)
 
+    if args.add_function:
+        add_function_to_yaml(yaml_file, args.add_function)
+
     header_str = str(header)
+    header_class = GpuHeader if args.export_decls else HeaderFile
+    header = load_yaml_file(args.yaml_file, header_class, args.entry_points)
 
-    if args.output_dir:
-        output_file_path = Path(args.output_dir)
-        if output_file_path.is_dir():
-            output_file_path /= f"{Path(args.yaml_file).stem}.h"
-    else:
-        output_file_path = Path(f"{Path(args.yaml_file).stem}.h")
+    header_str = str(header)
 
     if not args.export_decls and args.h_def_file:
         with open(args.h_def_file, "r") as f:
             h_def_content = f.read()
         final_header_content = fill_public_api(header_str, h_def_content)
-        with open(output_file_path, "w") as f:
-            f.write(final_header_content)
     else:
-        with open(output_file_path, "w") as f:
-            f.write(header_str)
+        final_header_content = str
+    write_to_file(output_file_path, final_header_content)
 
 
 if __name__ == "__main__":

michaelrj-google

Code changes overall LGTM, make sure to update the docs where necessary: https://github.com/llvm/llvm-project/blob/main/libc/docs/dev/header_generation.rst?plain=1

libc/newhdrgen/yaml_to_classes.py

frobtech · 2024-10-30T21:18:24Z

Code changes overall LGTM, make sure to update the docs where necessary: https://github.com/llvm/llvm-project/blob/main/libc/docs/dev/header_generation.rst?plain=1

That does not currently include any explanation of the file-related command line switches. It's not really documentation for the tool as a tool. It's more a few examples of things to try and what might happen.

fabio-d

I think it'd be great if the two blocks that decide yaml_file and output_file_path had some comment explaining the various cases with examples, because the code alone was a bit hard to follow to me

fabio-d · 2024-10-31T18:41:32Z

libc/newhdrgen/yaml_to_classes.py

    else:
-        with open(output_file_path, "w") as f:
-            f.write(header_str)
+        final_header_content = str


final_header_content = header_str?

fabio-d · 2024-10-31T18:42:28Z

libc/newhdrgen/yaml_to_classes.py

    header_class = GpuHeader if args.export_decls else HeaderFile
    header = load_yaml_file(args.yaml_file, header_class, args.entry_points)

+    if args.add_function:


Is this repetition (see 5 lines above) on purpose?

…drgen

michaelrj-google

Overall LGTM, but you need to fix the paths before landing.

michaelrj-google · 2024-12-10T18:55:04Z

libc/hdrgen/yaml_to_classes.py

+        output_file_path = args.output_dir
+        if output_file_path.is_dir():
+            if not args.yaml_file:
+                libc_yaml_dir = args.libc_dir / "newhdrgen" / "yaml"


also needs to be updated.

michaelrj-google · 2024-12-10T18:55:13Z

libc/hdrgen/yaml_to_classes.py

+    yaml_file = args.yaml_file
+    if args.h_def_file and not yaml_file:
+        libc_include_dir = args.libc_dir / "include"
+        libc_yaml_dir = args.libc_dir / "newhdrgen" / "yaml"


this needs to be updated since headergen has been moved to the hdrgen directory.

michaelrj-google · 2024-12-10T19:07:12Z

libc/hdrgen/yaml_to_classes.py

+    def write_to_file(path, contents):
+        if (
+            not args.write_if_changed
+            or not path.exists()
+            or path.read_text() != contents
+        ):
+            path.write_text(contents)


given that this is only used in one place at the moment, would it make more sense to inline this function?

frobtech · 2024-12-28T08:15:52Z

I'm going to abandon this and do some more basic refactoring to get towards a good CLI for both programmatic and human use. It will be in several smaller steps.

frobtech added 3 commits October 30, 2024 10:23

Restore old behavior for test suite with explicit YAML file arg

712bbb8

python formatting

15c13eb

Merge branch 'main' into p/libc-newhdrgen

1f75c67

frobtech requested review from Caslyn, fabio-d and michaelrj-google October 30, 2024 19:42

frobtech marked this pull request as ready for review October 30, 2024 19:43

llvmbot added the libc label Oct 30, 2024

michaelrj-google reviewed Oct 30, 2024

View reviewed changes

libc/newhdrgen/yaml_to_classes.py Show resolved Hide resolved

frobtech requested a review from michaelrj-google October 31, 2024 18:18

fabio-d requested changes Oct 31, 2024

View reviewed changes

frobtech added 2 commits December 5, 2024 13:17

Merge remote-tracking branch 'origin/main' into p/libc-newhdrgen

8475db2

Merge remote-tracking branch 'fork/p/libc-newhdrgen' into p/libc-newh…

44040ef

…drgen

michaelrj-google approved these changes Dec 10, 2024

View reviewed changes

frobtech closed this Dec 28, 2024

frobtech deleted the p/libc-newhdrgen branch February 14, 2025 19:55

[libc] Add some integration features into newhdrgen #114272

[libc] Add some integration features into newhdrgen #114272

Uh oh!

Conversation

frobtech commented Oct 30, 2024

Uh oh!

github-actions bot commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 30, 2024

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frobtech commented Oct 30, 2024

Uh oh!

fabio-d left a comment

Choose a reason for hiding this comment

Uh oh!

fabio-d Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

fabio-d Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

michaelrj-google Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

michaelrj-google Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

michaelrj-google Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

frobtech commented Dec 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Oct 30, 2024 •

edited

Loading