Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23301019:
Remove third_party/cld2 once (Closed)
|
Created:
7 years, 4 months ago by hajimehoshi Modified:
7 years, 3 months ago CC:
chromium-reviews, Takashi Toyoshima, cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemove third_party/cld2 once
I aim to change the directory where CLD is fetched to 'third_party/cld2/src', and this CL is the first step to perform that.
BUG=240647
Patch Set 1 #Patch Set 2 : Modify DEPS to remove the directory #Patch Set 3 : Replace 'rm' with a python script (we can't use 'rm' on Windows) #
Total comments: 4
Patch Set 4 : nits #
Total comments: 15
Patch Set 5 : (rebasing) #Patch Set 6 : nits #Messages
Total messages: 16 (0 generated)
Can you take a look? Thanks.
This is just reverting r218252, and I'll commit this with TBR. On 2013/08/22 06:00:03, hajimehoshi wrote: > Can you take a look? Thanks.
This isn't going to be sufficient -- gclient does not automatically delete unused directories. You will need a hook if you really want to make sure it gets deleted. And it should probably be in the same CL where you add the new path. Why do you need to move this?
Do not TBR structural DEPS changes. Ever.
Sorry. On 2013/08/22 06:03:40, Isaac wrote: > Do not TBR structural DEPS changes. Ever.
I mean, the revert isn't going to break anything. But it won't solve your problem. But please don't TBR stuff like this :-)
As we discussed, I added a hook to remove the existing directory of CLD2. Isaac, can you take a look? BTW, I'm still not sure our plan is safe for the perf bisect script, which is discussed in the thread of chromium-dev. On 2013/08/22 06:04:41, Isaac wrote: > I mean, the revert isn't going to break anything. But it won't solve your > problem. > > But please don't TBR stuff like this :-)
I have some style nits about this python script; adding M-A about whether this is feasible. https://codereview.chromium.org/23301019/diff/7001/DEPS File DEPS (right): https://codereview.chromium.org/23301019/diff/7001/DEPS#newcode626 DEPS:626: "action": ["python", "src/build/util/remove_file.py", "src/third_party/cld2"], I think 'remove_tree' would be more accurate. https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py File build/util/remove_file.py (right): https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py#... build/util/remove_file.py:13: sys.path.append(os.path.join(os.path.dirname(__file__), 'lib', 'common')) Why not use shutil?
Thanks! We want to start integrating CLD2 at 9/2 at the latest. We need to commit this CL asap in order to have enough time for the developers to delete CLD2. IMO, tomorrow is the dead line. For the case that committing this CL is so late that we can't start integrating then, we have an alternative plan, that is, moving CLD2 to another position 'third_party/cld/2/src' instead of 'third_party/cld2/src'. 'third_party/cld' is now managed under not an external repository but the chromium repository, and we think it is the easiest way to move a third-party library. https://codereview.chromium.org/23301019/diff/7001/DEPS File DEPS (right): https://codereview.chromium.org/23301019/diff/7001/DEPS#newcode626 DEPS:626: "action": ["python", "src/build/util/remove_file.py", "src/third_party/cld2"], On 2013/08/26 23:17:39, Isaac wrote: > I think 'remove_tree' would be more accurate. Done. https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py File build/util/remove_file.py (right): https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py#... build/util/remove_file.py:13: sys.path.append(os.path.join(os.path.dirname(__file__), 'lib', 'common')) On 2013/08/26 23:17:39, Isaac wrote: > Why not use shutil? Done.
BTW, dsites@ suggested to upgrade the version of CLD2 for some security fix. Is it possible to change the version now, or we should do this later? On 2013/08/27 03:40:50, hajimehoshi wrote: > Thanks! > > We want to start integrating CLD2 at 9/2 at the latest. We need to commit this > CL asap in order to have enough time for the developers to delete CLD2. IMO, > tomorrow is the dead line. > > For the case that committing this CL is so late that we can't start integrating > then, we have an alternative plan, that is, moving CLD2 to another position > 'third_party/cld/2/src' instead of 'third_party/cld2/src'. 'third_party/cld' is > now managed under not an external repository but the chromium repository, and we > think it is the easiest way to move a third-party library. > > https://codereview.chromium.org/23301019/diff/7001/DEPS > File DEPS (right): > > https://codereview.chromium.org/23301019/diff/7001/DEPS#newcode626 > DEPS:626: "action": ["python", "src/build/util/remove_file.py", > "src/third_party/cld2"], > On 2013/08/26 23:17:39, Isaac wrote: > > I think 'remove_tree' would be more accurate. > > Done. > > https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py > File build/util/remove_file.py (right): > > https://codereview.chromium.org/23301019/diff/7001/build/util/remove_file.py#... > build/util/remove_file.py:13: > sys.path.append(os.path.join(os.path.dirname(__file__), 'lib', 'common')) > On 2013/08/26 23:17:39, Isaac wrote: > > Why not use shutil? > > Done.
We (the Translate team Tokyo) decided to postpone the deadline and make an effort to continue the plan. I hope this can be committed in this week. Here is the plan I posted chromium-dev before: - step 1: remove cld2/ from DEPS and add a python hook to actually delete that path. (this CL) - step 2: wait a week to make sure most people have the fix. At the starting of this step, I'll announce about this. - step 3: add it back in the right place (cld2/src/). M-A, Would you take a look? Thanks.
more python nits. https://codereview.chromium.org/23301019/diff/23001/DEPS File DEPS (right): https://codereview.chromium.org/23301019/diff/23001/DEPS#newcode626 DEPS:626: "action": ["python", "src/build/util/remove_tree.py", "src/third_party/cld2"], Are you sure this works on windows? (it might, gyp might reconfigure paths; I don't know) https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py File build/util/remove_tree.py (right): https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:14: def main(argv=None): make option required https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:16: argv = sys.argv remove these lines https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:18: if len(argv) != 2: return 'Usage: %s <PATH>' % os.path.basename(__file__) https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:21: sys.exit(1) remove these lines https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:26: return 0 Not needed, sys.exit(None) exits with zero, remove https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:29: sys.exit(main()) pass sys.argv here
https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py File build/util/remove_tree.py (right): https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:18: if len(argv) != 2: Actually you should use: return 'Usage: %s <PATH>' % argv[0]
Thank you! https://codereview.chromium.org/23301019/diff/23001/DEPS File DEPS (right): https://codereview.chromium.org/23301019/diff/23001/DEPS#newcode626 DEPS:626: "action": ["python", "src/build/util/remove_tree.py", "src/third_party/cld2"], This seems working well on Windows according to the try server (http://build.chromium.org/p/tryserver.chromium/builders/win7_aura/builds/7226...). I'll try to run this on my local Windows machine when I have time. On 2013/08/28 06:22:56, Isaac wrote: > Are you sure this works on windows? (it might, gyp might reconfigure paths; I > don't know) https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py File build/util/remove_tree.py (right): https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:14: def main(argv=None): On 2013/08/28 06:22:56, Isaac wrote: > make option required Done. https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:16: argv = sys.argv On 2013/08/28 06:22:56, Isaac wrote: > remove these lines Done. https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:18: if len(argv) != 2: On 2013/08/28 06:24:18, Isaac wrote: > Actually you should use: > > return 'Usage: %s <PATH>' % argv[0] > Done. https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:21: sys.exit(1) On 2013/08/28 06:22:56, Isaac wrote: > remove these lines Done. https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:26: return 0 On 2013/08/28 06:22:56, Isaac wrote: > Not needed, sys.exit(None) exits with zero, remove Done. https://codereview.chromium.org/23301019/diff/23001/build/util/remove_tree.py... build/util/remove_tree.py:29: sys.exit(main()) On 2013/08/28 06:22:56, Isaac wrote: > pass sys.argv here Done.
And what about breaking reverse sync? E.g. sync back from revision-post-this-CL to reivison-before-this-CL. I think you should go the "rename the directory" way.
On 2013/08/28 14:56:54, M-A Ruel wrote: > And what about breaking reverse sync? E.g. sync back from revision-post-this-CL > to reivison-before-this-CL. I think you should go the "rename the directory" > way. Hmm, it could be that CLD2 is fetched at 'third_party/cld2/src' at a future revision and this remains at a revision-before-this-CL, which causes conflict. OK, we'll adopt the "renaming the directory" way. Maybe the new path will be "third_party/cld_2/src". Thank you for your advice. BTW, we realized that 'third_party/cld/2/src/...' is also dangerous because some tools including license.py don't consider nested third-party libraries even though 'third_party/cld' is managed under the Chromium repository. We need to adopt a completely separated new directory. |
