Page MenuHomePhabricator

[gn build] Move .gn file to the root of the monorepo
AbandonedPublic

Authored by thakis on Mon, Jan 7, 3:40 PM.

Details

Reviewers
phosek
Summary

This adds it to llvm.org/svn/llvm-project/monorepo-root/trunk which is mirrored to the root of https://github.com/llvm-git-prototype/llvm.

Having the .gn file in the root of the repo greatly improves the ergonomics of running gn: It's now just gn gen out/foo instead of gn --dotfile=llvm/utils/gn/.gn --root=. gen out/foo (likewise for all other gn commands).

I'll then remove it from llvm/utils/gn/.gn in a separate patch once everyone uses the official monorepo.

Diff Detail

Event Timeline

thakis created this revision.Mon, Jan 7, 3:40 PM
brzycki added a subscriber: brzycki.Mon, Jan 7, 4:03 PM

I am strongly against this commit. @thakis when this was discussed on llvm-dev the original proposal was the compromise of placing .gn in utils to signify that it was not the default, recommended, method to build LLVM. With this change LLVM will truly have two, parallel build systems once again.

In my opinion making the invocation of gn less cumbersome for a handful of Google developers does not justify the significance of having two build systems at the top level.

Can't this simply be solved with the following scripting?

git clone monorepo $DIR
cd $DIR
ln -s utils/.gn

I am strongly against this commit. @thakis when this was discussed on llvm-dev the original proposal was the compromise of placing .gn in utils to signify that it was not the default, recommended, method to build LLVM. With this change LLVM will truly have two, parallel build systems once again.

This is not changing. The first line in the added file here is: "# LLVM's GN build is unsupported, see llvm/utils/gn/README.rst for details." Is that not clear enough? I'm happy to repeat that line 100 times and add another 100 lines whitespace to make it more obvious.

I'm done upstreaming the stuff I had and as promised all changes except for one test change (r350416) have been in llvm/utils/gn.

I'd love to have this file at the toplevel. If there's strong pushback, I won't pursue it of course, but I don't see the downside of adding a hidden file at the toplevel yet.

In my opinion making the invocation of gn less cumbersome for a handful of Google developers

I'd like to point out that there's no interest from Google in this. It is true that I am employed by Google and that I wrote this for my personal use, and that most people I talk to socially about this are at Google, but there's no org interest in this. The participants at the GN roundtable who expressed interest in this were pretty mixed organizationally (maybe 40% Google employees). I'm not sure if this point matters much though.

does not justify the significance of having two build systems at the top level.

git clone monorepo $DIR
cd $DIR
ln -s utils/.gn

Sure, this works, but it's less convenient. The whole point of having this file here is convenience -- the symlink isn't needed either if you pass in --dotfile=llvm/utils/gn/.gn --root=. gen, as the change description mentions.

This is not changing. The first line in the added file here is: "# LLVM's GN build is unsupported, see llvm/utils/gn/README.rst for details." Is that not clear enough? I'm happy to repeat that line 100 times and add another 100 lines whitespace to make it more obvious.

If it is unsupported it should not be at the top-level. If it is moved to the top-level is should be supported. The playground for experimental and unsupported tools is utils/.

I don't see the downside of adding a hidden file at the toplevel yet.

The change promotes an unsupported, parallel, build system to the top of LLVM. It's precisely the situation I and others who spoke up on llvm-dev were trying to avoid.

I'd like to point out that there's no interest from Google in this.

So why make the change at all then if so few people are even using gn at all?

I don't see the downside of adding a hidden file at the toplevel yet.

The change promotes an unsupported, parallel, build system to the top of LLVM. It's precisely the situation I and others who spoke up on llvm-dev were trying to avoid.

Sorry, I don't see how a hidden file that says "this is unsupported" has much promotional value.

I'd like to point out that there's no interest from Google in this.

So why make the change at all then if so few people are even using gn at all?

Because people asked me for it, mostly. (I'll admit it's also more pleasant for myself to use now that it's upstream, but it was a lot of work to get it upstreamed, so it wouldn't have been worth it just for that.) Now that it's there I'm trying to make it as easy to use as possible, in the hope that that entices more people giving it a try, hence this change. (And why do I bother with that? So that more people hopefully see how needlessly slow the CMake build is, so that folks trying to make the CMake build get less pushback.)

I'm neutral on the issue but I would urge Nico to really think this through, lashback from some major downstream Linux distro vendors last time was pretty nasty, including flat out dropping Clang/LLVM support if CMake became a second class citizen. Is rehashing the same thing again worth it? Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake, not many are familiar with it outside Google or Chromium/Skia contributors.

I'm neutral on the issue but I would urge Nico to really think this through, lashback from some major downstream Linux distro vendors last time was pretty nasty, including flat out dropping Clang/LLVM support if CMake became a second class citizen. Is rehashing the same thing again worth it? Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake, not many are familiar with it outside Google or Chromium/Skia contributors.

I'd like to reiterate that this patch does not change anything about LLVM's build system situation. CMake is the supported build system. GN is 100% unsupported (as I've added as a comment to the file as I moved it over). I do not intend to push for changing this.

I don't have this in my notes, but my recollection from the GitHub round-table at the Dev Meeting was that the preference was to not have any files to the root directory. I think step 1 would be to discuss the general issue of files in the root directory on llvm-devel.

Sorry, I don't see how a hidden file that says "this is unsupported" has much promotional value.

Placing .gn at the top-level with a comment that it's unsupported contradicts users expectations. To the casual user outside the day-to-day of llvm-dev they have no idea if they should or should not use .gn. It's confusing at best: is the .gn file location stale or the comment stale or is something else going on? I've worked in this industry long enough to see too many unsupported tools quickly become the cornerstone of key infrastructure.

I genuinely don't understand why making a single symlink satisfies all potential use cases for the intrepid users who decide to build a compiler with an unsupported build system.

Now that it's there I'm trying to make it as easy to use as possible, in the hope that that entices more people giving it a try, hence this change.

One symlink step in the readme, or two parameters to gn isn't easy enough? I'm sure those that are curious to try it won't be discouraged by such small steps. Why entice users to try an unsupoorted build system? Keeping everything under utils/ sends a strong message to everyone: use this at your own risk.

This statement:

And why do I bother with that? So that more people hopefully see how needlessly slow the CMake build is, so that folks trying to make the CMake build get less pushback.

And this one:

GN is 100% unsupported (as I've added as a comment to the file as I moved it over). I do not intend to push for changing this.

seem like contradictions to me. Why bother challenging the existing CMake build system as "needlessly slow" if you don't intend to push for changing the build system?

This isn't just about gn: if this was a commit to put a scons file at the top level I'd be pushing back just as hard.

I've said my peace and I'll let other developers weigh in on this. I am still -1 on this commit.

kristina added a comment.EditedMon, Jan 7, 5:45 PM

I'm neutral on the issue but I would urge Nico to really think this through, lashback from some major downstream Linux distro vendors last time was pretty nasty, including flat out dropping Clang/LLVM support if CMake became a second class citizen. Is rehashing the same thing again worth it? Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake, not many are familiar with it outside Google or Chromium/Skia contributors.

I'd like to reiterate that this patch does not change anything about LLVM's build system situation. CMake is the supported build system. GN is 100% unsupported (as I've added as a comment to the file as I moved it over). I do not intend to push for changing this.

Again, I'm not against it (but I'm not in favor of it either), I only raised a point, it's not me you have to reassure. I'll emphasize what I said before - "Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake".

I'd be interested in feedback from more people if anyone has any (e.g. "don't care"). Given the current feedback, I'll put this on hold for now.

Sorry, I don't see how a hidden file that says "this is unsupported" has much promotional value.

Placing .gn at the top-level with a comment that it's unsupported contradicts users expectations. To the casual user outside the day-to-day of llvm-dev they have no idea if they should or should not use .gn. It's confusing at best: is the .gn file location stale or the comment stale or is something else going on? I've worked in this industry long enough to see too many unsupported tools quickly become the cornerstone of key infrastructure.

I genuinely don't understand why making a single symlink satisfies all potential use cases for the intrepid users who decide to build a compiler with an unsupported build system.

  • it's another setup step
  • no symlinks on windows without opting in to devmode

This statement:

And why do I bother with that? So that more people hopefully see how needlessly slow the CMake build is, so that folks trying to make the CMake build get less pushback.

And this one:

GN is 100% unsupported (as I've added as a comment to the file as I moved it over). I do not intend to push for changing this.

seem like contradictions to me. Why bother challenging the existing CMake build system as "needlessly slow" if you don't intend to push for changing the build system?

Because it's also possible to make the cmake build better, given that there's agreement that the current speed is a problem. So far there seems to be no appetite for that.

This isn't just about gn: if this was a commit to put a scons file at the top level I'd be pushing back just as hard.

I've said my peace and I'll let other developers weigh in on this. I am still -1 on this commit.

Thanks, that sounds fair.

chapuni added a subscriber: chapuni.Tue, Jan 8, 2:25 PM
dmajor added a subscriber: dmajor.Tue, Jan 8, 3:10 PM

I'd be interested in feedback from more people if anyone has any (e.g. "don't care").

I assume that nobody wants the GN build support out of a purely abstract interest or a desire to just play around with it once. Presumably, people (myself included) want to actually use it day to day, no matter how many times it says unofficial and unsupported. I don't think this can be avoided with disclaimers alone.

I can definitely understand how this would be cause for concern among people who don't want to see a parallel build system in the project. And I can definitely understand how elevating the GN support to a more official-looking and enticing place at the root of the tree would make the feeling worse. Holding off on this sounds like the right thing to me.

thakis abandoned this revision.Wed, Jan 9, 4:53 AM

Ok, I've abandoned this revision. Thanks for the feedback, everyone!