Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224093003:
Drop third_party/swiftshader/ from .gitignore (Closed)
DescriptionDrop third_party/swiftshader/ from .gitignore
It's no longer in DEPS, and its precense in .gitignore may be why it's
still left in the checkout causing checklicenses failures.
BUG=635880
TBR=stevenjb@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/341fac67de9500a95d8ed16545a9b0e1f859991d
Patch Set 1 #
Messages
Total messages: 16 (5 generated)
Message was sent while issue was closed.
Description was changed from ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org ========== to ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org Committed: https://crrev.com/341fac67de9500a95d8ed16545a9b0e1f859991d Cr-Commit-Position: refs/heads/master@{#410647} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/341fac67de9500a95d8ed16545a9b0e1f859991d Cr-Commit-Position: refs/heads/master@{#410647}
Message was sent while issue was closed.
Description was changed from ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org Committed: https://crrev.com/341fac67de9500a95d8ed16545a9b0e1f859991d Cr-Commit-Position: refs/heads/master@{#410647} ========== to ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/341fac67de9500a95d8ed16545a9... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 341fac67de9500a95d8ed16545a9b0e1f859991d (presubmit successful).
Message was sent while issue was closed.
On 2016/08/09 at 13:17:39, foolip wrote: > Committed patchset #1 (id:1) manually as 341fac67de9500a95d8ed16545a9b0e1f859991d (presubmit successful). Any chance we could revert this again now since the DEPS change was relanded in https://chromium.googlesource.com/chromium/src.git/+/e3976c2e7d2ce17180ebf6d2... ? (All those untracked files are a bit of a nuisance...)
Message was sent while issue was closed.
foolip@chromium.org changed reviewers: + capn@chromium.org
Message was sent while issue was closed.
capn@, can you take care of this? I did this as sheriff to fix a builder, and if swiftshader DEPS need to be reverted again it needs to happen together with .gitignore.
Message was sent while issue was closed.
Description was changed from ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/341fac67de9500a95d8ed16545a9... ========== to ========== Drop third_party/swiftshader/ from .gitignore It's no longer in DEPS, and its precense in .gitignore may be why it's still left in the checkout causing checklicenses failures. BUG=635880 TBR=stevenjb@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/341fac67de9500a95d8ed16545a9... ==========
Message was sent while issue was closed.
capn@chromium.org changed reviewers: + sugoi@chromium.org
Message was sent while issue was closed.
On 2016/08/15 14:29:26, foolip wrote: > capn@, can you take care of this? I did this as sheriff to fix a builder, and if > swiftshader DEPS need to be reverted again it needs to happen together with > .gitignore. Yes, I'll take care of anything to properly get SwiftShader into Chromium, but I'm slightly confused about what needs to happen here. Now that the SwiftShader source code is in //third_party/swiftshader (the first attempt caused build issues, which resulted in a revert and then caused checklicense failures, but the second attempt seems to stick), shouldn't it not be in .gitignore? That is, leave things the way they are now? Or is this just to inform me that if another revert were needed, that /third_party/swiftshader/ needs to be added to .gitignore again? Anyway, I'm currently doing a checkout from scratch to make sure everything looks ok...
Message was sent while issue was closed.
On 2016/08/15 15:29:59, capn wrote: > Anyway, I'm currently doing a checkout from scratch to make sure everything > looks ok... I see there's LICENSE and README.chromium which are untracked and aren't part of the new upstream SwiftShader repo. I'll look into where they come from and remove them.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2250003002/ by pkasting@chromium.org. The reason for reverting is: This was re-added to DEPS and is now showing up as untracked again..
Message was sent while issue was closed.
On 2016/08/15 15:29:59, capn wrote: > On 2016/08/15 14:29:26, foolip wrote: > > capn@, can you take care of this? I did this as sheriff to fix a builder, and > if > > swiftshader DEPS need to be reverted again it needs to happen together with > > .gitignore. > > Yes, I'll take care of anything to properly get SwiftShader into Chromium, but > I'm slightly confused about what needs to happen here. Now that the SwiftShader > source code is in //third_party/swiftshader (the first attempt caused build > issues, which resulted in a revert and then caused checklicense failures, but > the second attempt seems to stick), shouldn't it not be in .gitignore? That is, > leave things the way they are now? No. The source is pulled into those directories via DEPS and is not checked into them inside the Chromium repo. Therefore, from git's perspective, the entire directory is untracked. In other words, third_party/XXX/ should be in .gitignore iff it is in DEPS. It seems like your understanding is reversed from that. I went ahead and created the revert here.
Message was sent while issue was closed.
On 2016/08/16 03:22:28, Peter Kasting wrote: > On 2016/08/15 15:29:59, capn wrote: > > On 2016/08/15 14:29:26, foolip wrote: > > > capn@, can you take care of this? I did this as sheriff to fix a builder, > and > > if > > > swiftshader DEPS need to be reverted again it needs to happen together with > > > .gitignore. > > > > Yes, I'll take care of anything to properly get SwiftShader into Chromium, but > > I'm slightly confused about what needs to happen here. Now that the > SwiftShader > > source code is in //third_party/swiftshader (the first attempt caused build > > issues, which resulted in a revert and then caused checklicense failures, but > > the second attempt seems to stick), shouldn't it not be in .gitignore? That > is, > > leave things the way they are now? > > No. The source is pulled into those directories via DEPS and is not checked > into them inside the Chromium repo. Therefore, from git's perspective, the > entire directory is untracked. > > In other words, third_party/XXX/ should be in .gitignore iff it is in DEPS. It > seems like your understanding is reversed from that. > > I went ahead and created the revert here. Ah, I see, these repos are nested. It thought they were separate, like how Android handles external repos. Is it too late to suggest a directory structure reorg? Anyway, thanks for taking the right action here and explaining the core issue.
Message was sent while issue was closed.
On 2016/08/16 14:05:58, capn wrote: > On 2016/08/16 03:22:28, Peter Kasting wrote: > > On 2016/08/15 15:29:59, capn wrote: > > > On 2016/08/15 14:29:26, foolip wrote: > > > > capn@, can you take care of this? I did this as sheriff to fix a builder, > > and > > > if > > > > swiftshader DEPS need to be reverted again it needs to happen together > with > > > > .gitignore. > > > > > > Yes, I'll take care of anything to properly get SwiftShader into Chromium, > but > > > I'm slightly confused about what needs to happen here. Now that the > > SwiftShader > > > source code is in //third_party/swiftshader (the first attempt caused build > > > issues, which resulted in a revert and then caused checklicense failures, > but > > > the second attempt seems to stick), shouldn't it not be in .gitignore? That > > is, > > > leave things the way they are now? > > > > No. The source is pulled into those directories via DEPS and is not checked > > into them inside the Chromium repo. Therefore, from git's perspective, the > > entire directory is untracked. > > > > In other words, third_party/XXX/ should be in .gitignore iff it is in DEPS. > It > > seems like your understanding is reversed from that. > > > > I went ahead and created the revert here. > > Ah, I see, these repos are nested. It thought they were separate, like how > Android handles external repos. Is it too late to suggest a directory structure > reorg? Anyway, thanks for taking the right action here and explaining the core > issue. What kind of reorg would you propose? I'm not familiar with how Android does things. We pull a lot of external repos into various third_party/ directories via DEPS and put them in .gitignore, so this situation is fairly common.
Message was sent while issue was closed.
On 2016/08/16 18:48:01, Peter Kasting wrote: > On 2016/08/16 14:05:58, capn wrote: > > On 2016/08/16 03:22:28, Peter Kasting wrote: > > > On 2016/08/15 15:29:59, capn wrote: > > > > On 2016/08/15 14:29:26, foolip wrote: > > > > > capn@, can you take care of this? I did this as sheriff to fix a > builder, > > > and > > > > if > > > > > swiftshader DEPS need to be reverted again it needs to happen together > > with > > > > > .gitignore. > > > > > > > > Yes, I'll take care of anything to properly get SwiftShader into Chromium, > > but > > > > I'm slightly confused about what needs to happen here. Now that the > > > SwiftShader > > > > source code is in //third_party/swiftshader (the first attempt caused > build > > > > issues, which resulted in a revert and then caused checklicense failures, > > but > > > > the second attempt seems to stick), shouldn't it not be in .gitignore? > That > > > is, > > > > leave things the way they are now? > > > > > > No. The source is pulled into those directories via DEPS and is not checked > > > into them inside the Chromium repo. Therefore, from git's perspective, the > > > entire directory is untracked. > > > > > > In other words, third_party/XXX/ should be in .gitignore iff it is in DEPS. > > It > > > seems like your understanding is reversed from that. > > > > > > I went ahead and created the revert here. > > > > Ah, I see, these repos are nested. It thought they were separate, like how > > Android handles external repos. Is it too late to suggest a directory > structure > > reorg? Anyway, thanks for taking the right action here and explaining the core > > issue. > > What kind of reorg would you propose? I'm not familiar with how Android does > things. > > We pull a lot of external repos into various third_party/ directories via DEPS > and put them in .gitignore, so this situation is fairly common. Basically if third_party/ was next to src/ and each project was from a separate repository (instead of a bunch of them being in bundled in chromium/src), there wouldn't be a need to add ones from DEPS to .gitignore (in fact there wouldn't be a .gitignore at the top level). Anyway, besides involving a lot of work and getting critique from people used to the current structure, there would be a few downsides to switching to the way Android does things as well, like complicating bisect across multiple repositories. :-( So never mind. Having worked on Android for a while I was just confused about how Chromium's .gitignore could have anything to do with SwiftShader. Makes perfect sense now. It's even nicely mentioned in the docs, but, not clarified "why": https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Setting-up.... So I'll update that to avoid similar issues and confusion in the future. Thanks again. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
