[PATCH] D54345: Add initial scaffolding for the GN build.

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

Shouldn't all the GN files have a copyright header?



================
Comment at: llvm/utils/gn/.gn:9
+
+secondary_source = "//llvm/utils/gn/tree/"
+
----------------
The convention is to use the name `secondary`, i.e. `//llvm/utils/gn/secondary`.


================
Comment at: llvm/utils/gn/.gn:11
+
+root = "//llvm/utils/gn"
----------------
I prefer `llvm/contrib/gn` as suggested on the list over `utils`.


================
Comment at: llvm/utils/gn/README.rst:90
+
+GN believes in using gn arguments to configure the build explicitly, instead
+of implicitly figuring out what to do based on what's available on the current
----------------
nit: capitalize GN here and below


================
Comment at: llvm/utils/gn/build/BUILD.gn:6
+
+config("compiler_defaults") {
+  # FIXME: Don't define this globally here.
----------------
I'd prefer to break these into smaller configs so they can be added/removed independently rather than using nested if/else branches, but it's something that can be probably done later as a cleanup.


================
Comment at: llvm/utils/gn/build/buildflags.gni:9
+  # Whether to enable assertions.
+  llvm_enable_assertions = true
+}
----------------
Shouldn't this be `llvm_enable_assertions = is_debug`?


================
Comment at: llvm/utils/gn/build/mac_sdk.gni:4
+# retrieve this, but for now this seems to do the trick.
+mac_sdk = "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk"
----------------
Make it an argument so it can be overriden if needed? Also maybe call it `mac_sdk_path` so it's obvious that it's a path.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:3
+
+declare_args() {
+  # If is_goma is true, the location of the goma client install.
----------------
This should be moved to a separate `.gni` file, see below.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:12
+
+toolchain("posix") {
+  cc = "cc"
----------------
I'd prefer moving these into a template and then define the concrete instance using those templates like most GN builds do. I think it makes it easier to define different toolchains, e.g. for clang and gcc or clang-cl and cl.

Also `"posix"` is kind of a strange name, Fuchsia also uses the GCC style frontend but we're not POSIX :P I'd slightly prefer using the name Chromium convention to name these after the flag style, i.e. `gcc_toolchain` or `cl_toolchain`.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:47
+
+  tool("alink") {
+    if (host_os == "mac") {
----------------
We should use response files here to pass the list of inputs.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:49
+    if (host_os == "mac") {
+      command = "libtool -static -no_warning_for_no_symbols {{arflags}} -o {{output}} {{inputs}}"
+    } else {
----------------
Why not use `ar` on macOS as well?


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:51
+    } else {
+      command = "ar rcsDT {{arflags}} -o {{output}} {{inputs}}"
+    }
----------------
Can we make `ar` variable as well? I'd like to change this, e.g. for Clang toolchains to use `llvm-ar`.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:53
+    }
+    description = "LIBTOOL-STATIC {{output}}"
+    outputs = [
----------------
nit: I'd just use `AR` as the name.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:55
+    outputs = [
+      "{{output_dir}}/{{target_output_name}}.a",
+    ]
----------------
Can you also set `default_output_extension = ".a"` and use `{{target_output_name}}{{output_extension}}` here?


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:61
+
+  tool("solink") {
+    outfile = "{{output_dir}}/{{target_output_name}}{{output_extension}}"
----------------
Ditto here, use response files.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:65
+      command =
+          "$ld -shared {{ldflags}} -Wl,-z,defs -o $outfile {{libs}} {{inputs}}"
+    } else {
----------------
The `-Wl,-z,defs` should be handled through compiler config, not here.

You also want `{{solibs}}` beside `{{libs}}` here.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:73
+    ]
+    lib_switch = "-l"
+    output_prefix = "lib"
----------------
Why do you prefer this over specifying the library with a full path?


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:76
+    default_output_dir = "{{root_out_dir}}/lib"
+    if (host_os == "mac") {
+      default_output_extension = ".dylib"
----------------
nit: I'd combine this branch with the one above.


================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:103
+
+  tool("link") {
+    outfile = "{{output_dir}}/{{target_output_name}}{{output_extension}}"
----------------
Response file here as well.


================
Comment at: llvm/utils/gn/build/toolchain/compiler.gni:3
+  # Whether to use goma (https://chromium.googlesource.com/infra/goma/client/)
+  use_goma = false
+
----------------
Can we split this into a separate file, e.g. `goma.gni`? This is a one we use in Fuchsia: https://fuchsia.googlesource.com/build/+/master/toolchain/goma.gni


================
Comment at: llvm/utils/gn/tree/llvm/lib/Demangle/BUILD.gn:2
+static_library("Demangle") {
+  output_name = "LLVMDemangle"
+
----------------
Setting `output_name` is generally frowned upon as an anti-pattern that should be only used when absolutely necessary, I'd prefer to just set the name of the target to `LLVMDemangle`.


https://reviews.llvm.org/D54345