Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782923002:
Unbundle libdrm (Closed)
|
Created:
3 years, 8 months ago by Paweł Hajdan Jr. Modified:
3 years, 8 months ago CC:
chromium-reviews, phajdan Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnbundle libdrm
BUG=551343
R=thestig@chromium.org
Review-Url: https://codereview.chromium.org/2782923002 .
Cr-Commit-Position: refs/heads/master@{#460751}
Committed: https://chromium.googlesource.com/chromium/src/+/9a50f192a882b65f021c0c13345762c3a4d7a499
Patch Set 1 #Patch Set 2 : fix #
Messages
Total messages: 18 (4 generated)
Description was changed from ========== Unbundle libdrm BUG=none ========== to ========== Unbundle libdrm BUG=551343 ==========
phajdan.jr@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org, thestig@chromium.org, thomasanderson@chromium.org
thomasanderson@google.com changed reviewers: + dcastagna@chromium.org, thomasanderson@google.com
+dcastanga
Hi phajdan.jr! Do you mind following the suggested form for CL descriptions: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... In particular, when I look at a CL description I'd like to: - Understand what the change is about without need to manually dig for more info. - Understand why the change needs to be made. Looking at the linked bug, I see a title "metabug for GN Linux distro build" that doesn't add much and a list of CLs.
On 2017/03/29 at 18:40:32, Daniele Castagna wrote: > Hi phajdan.jr! > Do you mind following the suggested form for CL descriptions: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > > In particular, when I look at a CL description I'd like to: > - Understand what the change is about without need to manually dig for more info. > - Understand why the change needs to be made. > > Looking at the linked bug, I see a title "metabug for GN Linux distro build" that doesn't add much and a list of CLs. Is build/linux used on CrOS too? We're using third_party/libdrm only for specific use cases (building ozone unit tests on the bots where they have an old version of libdrm in the sysroot). When we build Chrome for CrOS we use the system libdrm. We're not shipping any binary including third_party/libdrm and IIRC only ozone builds (maybe only with gbm platform) depend on that.
Please see https://cs.chromium.org/chromium/src/build/linux/unbundle/README?q=unbundle/r... for more context. *NONE* of these files are used at Google.
On 2017/03/29 at 21:23:17, phajdan.jr wrote: > Please see https://cs.chromium.org/chromium/src/build/linux/unbundle/README?q=unbundle/r... for more context. > I think a CL description that gives a little bit more context than "Unbundle libdrm" would still be more useful for everyone. > *NONE* of these files are used at Google. I'm not sure why you're saying that. My comment was that libdrm might only be used for CrOS builds. If libdrm is not used anywhere else, and build/unbundle is not used on CrOS, I don't see why this change would be needed at all.
On 2017/03/29 21:30:25, Daniele Castagna wrote: > On 2017/03/29 at 21:23:17, phajdan.jr wrote: > > Please see > https://cs.chromium.org/chromium/src/build/linux/unbundle/README?q=unbundle/r... > for more context. > > > > I think a CL description that gives a little bit more context than "Unbundle > libdrm" would still be more useful for everyone. > > > *NONE* of these files are used at Google. > > I'm not sure why you're saying that. My comment was that libdrm might only be > used for CrOS builds. If libdrm is not used anywhere else, and build/unbundle is > not used on CrOS, I don't see why this change would be needed at all. On Linux, if I build the chrome target, then touch third_party/libdrm/src/xf86*.c, and rebuild the chrome target, I see libdrm files being rebuilt. Something somewhere is depending on it.
On 2017/03/30 at 06:35:41, thestig wrote: > On 2017/03/29 21:30:25, Daniele Castagna wrote: > > On 2017/03/29 at 21:23:17, phajdan.jr wrote: > > > Please see > > https://cs.chromium.org/chromium/src/build/linux/unbundle/README?q=unbundle/r... > > for more context. > > > > > > > I think a CL description that gives a little bit more context than "Unbundle > > libdrm" would still be more useful for everyone. > > > > > *NONE* of these files are used at Google. > > > > I'm not sure why you're saying that. My comment was that libdrm might only be > > used for CrOS builds. If libdrm is not used anywhere else, and build/unbundle is > > not used on CrOS, I don't see why this change would be needed at all. > > On Linux, if I build the chrome target, then touch third_party/libdrm/src/xf86*.c, and rebuild the chrome target, I see libdrm files being rebuilt. Something somewhere is depending on it. crrev.com/2711933002 seemed to have introduced the dependency five days ago. Btw, you can see who triggers the build with "ninja -C out/Debug/ -t browse libdrm".
On 2017/03/30 at 07:16:34, Daniele Castagna wrote: > On 2017/03/30 at 06:35:41, thestig wrote: > > On 2017/03/29 21:30:25, Daniele Castagna wrote: > > > On 2017/03/29 at 21:23:17, phajdan.jr wrote: > > > > Please see > > > https://cs.chromium.org/chromium/src/build/linux/unbundle/README?q=unbundle/r... > > > for more context. > > > > > > > > > > I think a CL description that gives a little bit more context than "Unbundle > > > libdrm" would still be more useful for everyone. > > > > > > > *NONE* of these files are used at Google. > > > > > > I'm not sure why you're saying that. My comment was that libdrm might only be > > > used for CrOS builds. If libdrm is not used anywhere else, and build/unbundle is > > > not used on CrOS, I don't see why this change would be needed at all. > > > > On Linux, if I build the chrome target, then touch third_party/libdrm/src/xf86*.c, and rebuild the chrome target, I see libdrm files being rebuilt. Something somewhere is depending on it. > > crrev.com/2711933002 seemed to have introduced the dependency five days ago. > > Btw, you can see who triggers the build with "ninja -C out/Debug/ -t browse libdrm". Looking at the comment, the added dependency seems intended. Can we just use the "use_system_libdrm" gn arg that is declared in libdrm.gn instead of this patch?
On 2017/03/30 07:22:17, Daniele Castagna wrote: > Looking at the comment, the added dependency seems intended. > Can we just use the "use_system_libdrm" gn arg that is declared in libdrm.gn > instead of this patch? Pawel can correct me if I'm wrong, but the idea is that Linux Chromium packagers can start with our source tarball, and then use the unbundle tool to: 1) remove the bundled third_party library 2) put in a substitute GN file Then they can build Chromium more normally without having to set a bunch of use_system_libfoo build flags, and they can create a new source tarball for their build that doesn't include all the bundled files that they will never use.
On 2017/03/30 07:22:17, Daniele Castagna wrote: > Can we just use the "use_system_libdrm" gn arg that is declared in libdrm.gn > instead of this patch? No. Not all GN files for third party libraries provide that, and when they do, it's not always what Linux distros expect/need. build/linux/unbundle provides a uniform way to unbundle libraries for Linux distros since 2013. They can also be freely modified without affecting anything used by Google. Please note build/linux/unbundle was not intended to be used by regular Chromium developers (as one of design requirements), and that only people who know the context would look there. In my experience, people need a lot of context if they don't already have it, and at that point a discussion (IM/VC?) might be best. Lei: why not just approve this CL? I'd expect you to be fairly familiar with build/linux/unbundle :)
On 2017/03/30 08:23:38, Paweł Hajdan Jr. wrote: > Lei: why not just approve this CL? I'd expect you to be fairly familiar with > build/linux/unbundle :) lgtm
Description was changed from ========== Unbundle libdrm BUG=551343 ========== to ========== Unbundle libdrm BUG=551343 R=thestig@chromium.org Review-Url: https://codereview.chromium.org/2782923002 . Cr-Commit-Position: refs/heads/master@{#460751} Committed: https://chromium.googlesource.com/chromium/src/+/9a50f192a882b65f021c0c133457... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 9a50f192a882b65f021c0c13345762c3a4d7a499 (presubmit successful).
Message was sent while issue was closed.
On 2017/03/30 08:23:38, Paweł Hajdan Jr. wrote: > No. Not all GN files for third party libraries provide that, and when they do, > it's not always what Linux distros expect/need. build/linux/unbundle provides a > uniform way to unbundle libraries for Linux distros since 2013. They can also be > freely modified without affecting anything used by Google. > > Please note build/linux/unbundle was not intended to be used by regular Chromium > developers (as one of design requirements), and that only people who know the > context would look there. In my experience, people need a lot of context if they > don't already have it, and at that point a discussion (IM/VC?) might be best. While it would be excessive to write a long paragraph for every unbundle CL, just "Unbundle libdrm" is on the opposite end of the spectrum. In general, we can provide a bit more context in the CL description, to point people in the right direction. Then hopefully they can figure it out and won't need to IM you as much. Just a cookie cutter "See build/linux/unbundle/README" can be really helpful. I'll also make a CL later to update the README to be more helpful for those without any context. For this CL in particular, third_party/libdrm has existed for a few months now. Did r459592 start using it on Linux, and prompted someone to file a Gentoo bug about unbundling it? If so, mentioning r459592 or some other bug specific to libdrm would have been helpful for reviewers. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
