Page MenuHomePhabricator

gn build: Add BPF target.
ClosedPublic

Authored by pcc on Jan 29 2019, 8:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Jan 29 2019, 8:33 PM
ast requested changes to this revision.Jan 29 2019, 8:57 PM
ast added a subscriber: ast.

thanks for doing this work.
any pointers to read about GN?
Also please include code owners as reviewers next time.

This revision now requires changes to proceed.Jan 29 2019, 8:57 PM
phosek accepted this revision.Jan 30 2019, 12:12 AM

LGTM

llvm/utils/gn/secondary/llvm/lib/Target/BPF/MCTargetDesc/BUILD.gn
39 ↗(On Diff #184246)

Nit: newline between targets.

This revision was not accepted when it landed; it landed in state Needs Revision.Jan 30 2019, 10:04 AM
Closed by commit rL352638: gn build: Add BPF target. (authored by pcc, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
pcc reopened this revision.EditedJan 30 2019, 11:49 AM

Re-opening since this was reverted in rL352649.

@yonghong-song this wasn't a change to the BPF backend so I'm not entirely clear on why you think this needed to be reviewed by a BPF code owner?

thakis accepted this revision.Jan 30 2019, 12:58 PM

lgtm too.

@ast @yonghong-song This code isn't in lib/Target/BPF and you're not taking on any maintenance of these new files. This is just a clone of the CMake build files; see git log llvm/utils/gn/secondary/llvm/lib/Target for several other files like this.

I read through the link and yes the code looks okay. I think this is a communication issue.
It would be good next time if you have better commit messages which provides enough info. about the change so reviewers can at least
take a look knowing what happens. In the case the reviewer requested a change, it would be good to send out information (as you did)
and wait for acknowledgement before merging.

yonghong-song accepted this revision.Jan 30 2019, 1:02 PM
ast added a comment.Jan 30 2019, 1:03 PM

hmm. so you're saying when we add new file to the normal CMakeFiles, we don't need to touch GN ?
So it's going to bit rot immediately?
what's the purpose of this GN work then?

In D57436#1377680, @ast wrote:

hmm. so you're saying when we add new file to the normal CMakeFiles, we don't need to touch GN ?

Yes.

So it's going to bit rot immediately?

No – turns out files aren't changed that frequently, and people who use the GN build usually fix up the build files pretty quickly. (git log --pretty=oneline --grep Merge llvm/utils/gn/)

what's the purpose of this GN work then?

gn runs 1000x faster than cmake, and it allows things like https://reviews.llvm.org/D56713 . It also has the effect of us reading the cmake files very closely and cleaning them up a bit :-)

ast accepted this revision.Jan 30 2019, 1:17 PM
In D57436#1377680, @ast wrote:

hmm. so you're saying when we add new file to the normal CMakeFiles, we don't need to touch GN ?

Yes.

ok. fair enough.

This revision is now accepted and ready to land.Jan 30 2019, 1:17 PM
Closed by commit rL352705: Reland "gn build: Add BPF target." (authored by pcc, committed by ). · Explain WhyJan 30 2019, 4:41 PM
This revision was automatically updated to reflect the committed changes.