[PATCH] D54345: Add initial scaffolding for the GN build.
Nico Weber via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 13 07:10:31 PST 2018
thakis added a comment.
Thanks for the thorough review! I replied "not needed yet" to several items, I hope that's fine.
I added a few comments to the .gn file, and I realized that I can put the root BUILD.gn file in gn/secondary and then I don't need to set root, so I did that.
> Shouldn't all the GN files have a copyright header?
Maybe! But all the cmake files don't have them either, so I'm not sure.
================
Comment at: llvm/utils/gn/.gn:9
+
+secondary_source = "//llvm/utils/gn/tree/"
+
----------------
phosek wrote:
> The convention is to use the name `secondary`, i.e. `//llvm/utils/gn/secondary`.
Done. I think guessing the meaning of a directory called "secondary" is super hard if you don't know about this one line of gn code here, and "tree" is much more self-explanatory, but /shrug.
================
Comment at: llvm/utils/gn/.gn:11
+
+root = "//llvm/utils/gn"
----------------
phosek wrote:
> I prefer `llvm/contrib/gn` as suggested on the list over `utils`.
I agree it'd be a good place if there was a contrib dir, but there isn't, so utils is a better place imo. (My reading of the thread is that someone said that some projects have a contrib dir and that would be a good place if it existed, and someone else saying that that might be a good place for a few other things too, but nobody really suggested putting this in a hypothetical contrib dir, since it doesn't exist yet.)
Anyhoo, this is a bit of a bikeshed and easy to change later if someone feels we need contrib in addition to tools and utils. For now, this location is as good as most others imho, and it's also one dir suggested in that thread :-)
================
Comment at: llvm/utils/gn/build/BUILD.gn:6
+
+config("compiler_defaults") {
+ # FIXME: Don't define this globally here.
----------------
phosek wrote:
> 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.
Ack. I'm happy with folks tweaking this to their heart's content later on.
================
Comment at: llvm/utils/gn/build/buildflags.gni:9
+ # Whether to enable assertions.
+ llvm_enable_assertions = true
+}
----------------
phosek wrote:
> Shouldn't this be `llvm_enable_assertions = is_debug`?
It's at least intentional; from README.rst: "By default, you get a release build with assertions enabled that targets the host arch". That's imho the best config for day-to-day development, which is what we're targeting here. I don't feel strongly about this, but it's imho the best default.
================
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"
----------------
phosek wrote:
> 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.
We can make it an argument once someone needs to override it. Until then, YAGNI :-)
But I did rename it, that's a better name.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:3
+
+declare_args() {
+ # If is_goma is true, the location of the goma client install.
----------------
phosek wrote:
> This should be moved to a separate `.gni` file, see below.
Replied below.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:12
+
+toolchain("posix") {
+ cc = "cc"
----------------
phosek wrote:
> 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`.
I expect that this will be necessary once we do cross builds, bootstrap builds, etc. Until then, I'd like to keep this as-is if that's alright – then blame history will show where the template came from and why it's there.
Re "posix": We used to have LLVM_ON_WIN32 and LLVM_ON_POSIX (we still have the latter), so these names were consistent with that. I don't care much about the name, and it's easy to change (referenced in 2 places I think).
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:47
+
+ tool("alink") {
+ if (host_os == "mac") {
----------------
phosek wrote:
> We should use response files here to pass the list of inputs.
LLVM is small enough that they aren't needed (at least for the parts I've hooked up), and if you can get away without them that's preferable: You can just copy failing link commands around without having to pass `-d keeprsp` to ninja, and it's easier to see the full link command.
================
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 {
----------------
phosek wrote:
> Why not use `ar` on macOS as well?
Last I checked (a few years ago), libtool -static was considerably faster than ar.
I just re-checked and it's still a tiny bit faster. It's _much_ faster if I don't rm the file first, which points out that the ar branch below doesn't rm first -- so thanks for the comment, the other branch had a bug :-) Fixed!
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:51
+ } else {
+ command = "ar rcsDT {{arflags}} -o {{output}} {{inputs}}"
+ }
----------------
phosek wrote:
> Can we make `ar` variable as well? I'd like to change this, e.g. for Clang toolchains to use `llvm-ar`.
Sure, but let's do that when we need it?
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:53
+ }
+ description = "LIBTOOL-STATIC {{output}}"
+ outputs = [
----------------
phosek wrote:
> nit: I'd just use `AR` as the name.
Sure, done.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:55
+ outputs = [
+ "{{output_dir}}/{{target_output_name}}.a",
+ ]
----------------
phosek wrote:
> Can you also set `default_output_extension = ".a"` and use `{{target_output_name}}{{output_extension}}` here?
I haven't found a place where I had to override this yet.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:61
+
+ tool("solink") {
+ outfile = "{{output_dir}}/{{target_output_name}}{{output_extension}}"
----------------
phosek wrote:
> Ditto here, use response files.
Same answer, don't need it (yet?) and more convenient to not use them when not needed.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:65
+ command =
+ "$ld -shared {{ldflags}} -Wl,-z,defs -o $outfile {{libs}} {{inputs}}"
+ } else {
----------------
phosek wrote:
> The `-Wl,-z,defs` should be handled through compiler config, not here.
>
> You also want `{{solibs}}` beside `{{libs}}` here.
I consider `-Wl,-z,defs` not being the default a bug :-) Unless something wants to disable that, it can stay here.
`{{solibs}}` isn't here because I haven't implemented the llvm-as-lots-of-shared-objects, and so nothing needs that atm.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:73
+ ]
+ lib_switch = "-l"
+ output_prefix = "lib"
----------------
phosek wrote:
> Why do you prefer this over specifying the library with a full path?
It's how I write my commands by hand (`cc foo.cc -lbar`), and it's what chromium uses. No strong preference though.
================
Comment at: llvm/utils/gn/build/toolchain/BUILD.gn:103
+
+ tool("link") {
+ outfile = "{{output_dir}}/{{target_output_name}}{{output_extension}}"
----------------
phosek wrote:
> Response file here as well.
(same answer)
================
Comment at: llvm/utils/gn/build/toolchain/compiler.gni:3
+ # Whether to use goma (https://chromium.googlesource.com/infra/goma/client/)
+ use_goma = false
+
----------------
phosek wrote:
> 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
If you use goma, you need to use some compiler locally that's available remotely, i.e. you'll have to set clang_base_path. So it's not clear to me if that should be in goma.gni or here. If it's in goma.gni, then I'd have to include that here for the is_clang default value below. If it's here, then I'd have to put an awkward "see clang_base_path in that other file" comment in goma.gni. So I figured one file is simpler, especially since it's just a single arg.
================
Comment at: llvm/utils/gn/tree/llvm/lib/Demangle/BUILD.gn:2
+static_library("Demangle") {
+ output_name = "LLVMDemangle"
+
----------------
phosek wrote:
> 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`.
I don't frown upon it :-)
The options here are:
1. Set the target name to LLVMDemangle. This means we'd have to refer to everything as "//llvm/lib/foo:LLVMFoo" instead of "//llvm/lib/foo", which is annoying.
2. Don't set output_name and have output files different from the cmake build. IIRC, llvm-config returns the names of these files and something tests for that, so that's kind of annoying too.
3. As-is, which has no downside other than apparently someone frowning upon it.
https://reviews.llvm.org/D54345
More information about the llvm-commits
mailing list