Issue 1304004: Update DEPS to bring in gtest, gmock and GYP. (Closed)
|
Created:
10 years, 7 months ago by chrisha Modified:
10 years, 7 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
https://chromium.googlesource.com/external/google-breakpad.git@master Visibility:
Public. |
DescriptionUpdate DEPS to bring in gtest, gmock and GYP. BUG=575 R=mark@chromium.org Committed: https://code.google.com/p/google-breakpad/source/detail?r=1297 Patch Set 1 #Patch Set 2 : Updated to reflect all svn:externals. #
Total comments: 11
Patch Set 3 : Addressed comments. #
Total comments: 4
Patch Set 4 : #MessagesTotal messages: 10
Breakpad doesn’t currently sync with gclient at all, it syncs using svn:externals. It’s not clear that you’re intending to change this, but if you are, you should also do something about the existing svn:externals.
Sign in to reply to this message.
On 2014/04/02 13:27:32, Mark Mentovai wrote: > Breakpad doesn’t currently sync with gclient at all, it syncs using > svn:externals. It’s not clear that you’re intending to change this, but if you > are, you should also do something about the existing svn:externals. Ah, thanks for the heads up. Yes, my intent is to replace svn:externals so that breakpad can be hacked on using git + gclient workflow as well. Are you onboard with replacing all of the svn:externals with equivalent DEPS?
Sign in to reply to this message.
Not without asking google-breakpad-dev. Breakpad previously didn’t require gclient or any other depot_tools. This will be a change that some Breakpad users may have opinions on, or at least need some time to adapt to.
Sign in to reply to this message.
On 2014/04/02 14:05:54, Mark Mentovai wrote: > Not without asking google-breakpad-dev. > > Breakpad previously didn’t require gclient or any other depot_tools. This will > be a change that some Breakpad users may have opinions on, or at least need some > time to adapt to. This now fully mirrors the current svn:externals mechanism, and is optional. It allows the use of a git checkout. I've send a message to the mailing list describing my proposed plan, and in the long term would like to remove svn:externals support and require use of gclient. PTAL?
Sign in to reply to this message.
https://breakpad.appspot.com/1304004/diff/50001/DEPS File DEPS (right): https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode29 DEPS:29: # This is used to mimic the svn:externals mechanism for Git based checkouts of Does gclient use --ignore-externals when checking out and updating over svn? If not, there’s an opportunity for collisions between things checked out by svn:external and things at the same path checked out by gclient. https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode39 DEPS:39: "lss_revision": "24", This is the only var we should have, and at that, it should only be a var if it’s referenced more than once (which I will argue against, see below). Everything else is only referenced once and would be more readable if simply written inline. https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode46 DEPS:46: "src/src/third_party/glog": What is this src/src foolishment? https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode67 DEPS:67: # Linux syscall support. LSS is very small and I don’t see the benefit of not checking it out on all platforms. I want to get the LSS checkout on my Mac, for example. Using svn:externals, you always get LSS regardless of OS.
Sign in to reply to this message.
PTAL https://breakpad.appspot.com/1304004/diff/50001/DEPS File DEPS (left): https://breakpad.appspot.com/1304004/diff/50001/DEPS#oldcode42 DEPS:42: "src/src/client/windows/breakpad_client.gyp"], The "src/src" foolishness was already present... https://breakpad.appspot.com/1304004/diff/50001/DEPS File DEPS (right): https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode29 DEPS:29: # This is used to mimic the svn:externals mechanism for Git based checkouts of On 2014/04/02 18:01:00, Mark Mentovai wrote: > Does gclient use --ignore-externals when checking out and updating over svn? If > not, there’s an opportunity for collisions between things checked out by > svn:external and things at the same path checked out by gclient. Yes, gclient uses --ignore-externals when grabbing SVN repositories. However, as is you shouldn't try running a 'gclient sync' inside of an SVN checkout, as the externals will already be in place. I've tried to make the comment more clear. (Also, gclient doesn't clobber directories that have already been checked out by the svn:externals mechanism; as long as the revisions and locations are kept in sync everything is happy.) https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode39 DEPS:39: "lss_revision": "24", On 2014/04/02 18:01:00, Mark Mentovai wrote: > This is the only var we should have, and at that, it should only be a var if > it’s referenced more than once (which I will argue against, see below). > Everything else is only referenced once and would be more readable if simply > written inline. Done. Force of habit from other projects. I personally like having a separate section with all of the revision information in one place, and find it easier to read. But, I don't care that much :) https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode46 DEPS:46: "src/src/third_party/glog": On 2014/04/02 18:01:00, Mark Mentovai wrote: > What is this src/src foolishment? This is due to a slight impedance mismatch with the current breakpad layout and gclient conventions (as used in Chromium projects). Paths in a DEPS file are relative the location of the .gclient file. Each repo in a .gclient file is given a name; by convention this is 'src' for the top-level project (often augmented with others repos like 'src-internal'). This is the source of the first 'src' in the path. We could choose to give this a different name if desired: google-breakpad? google_breakpad? breakpad? https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode67 DEPS:67: # Linux syscall support. On 2014/04/02 18:01:00, Mark Mentovai wrote: > LSS is very small and I don’t see the benefit of not checking it out on all > platforms. I want to get the LSS checkout on my Mac, for example. Using > svn:externals, you always get LSS regardless of OS. Done.
Sign in to reply to this message.
LGTM with this couple of comment changes. https://breakpad.appspot.com/1304004/diff/50001/DEPS File DEPS (left): https://breakpad.appspot.com/1304004/diff/50001/DEPS#oldcode42 DEPS:42: "src/src/client/windows/breakpad_client.gyp"], chrisha wrote: > The "src/src" foolishness was already present... Well, that sucks. We should clean this up. Separately, I guess. https://breakpad.appspot.com/1304004/diff/50001/DEPS File DEPS (right): https://breakpad.appspot.com/1304004/diff/50001/DEPS#newcode46 DEPS:46: "src/src/third_party/glog": chrisha wrote: > On 2014/04/02 18:01:00, Mark Mentovai wrote: > > What is this src/src foolishment? > > This is due to a slight impedance mismatch with the current breakpad layout and > gclient conventions (as used in Chromium projects). > > Paths in a DEPS file are relative the location of the .gclient file. Each repo > in a .gclient file is given a name; by convention this is 'src' for the > top-level project (often augmented with others repos like 'src-internal'). This > is the source of the first 'src' in the path. > > We could choose to give this a different name if desired: google-breakpad? > google_breakpad? breakpad? I guess google_breakpad. https://breakpad.appspot.com/1304004/diff/120001/DEPS File DEPS (right): https://breakpad.appspot.com/1304004/diff/120001/DEPS#newcode31 DEPS:31: # in an SVN checkout of the repository. If using an SVN checkout you can still “not intended” but if you want a gclient-managed svn checkout, it should work just fine. You should get rid of the “it is not intended” sentence and loosen the “if using an svn checkout” one to say “if using svn directly as opposed to via gclient sync”. https://breakpad.appspot.com/1304004/diff/120001/DEPS#newcode35 DEPS:35: # Cross-platform dependencies. All dependencies are cross-platform now, this comment doesn’t disambiguate anything and should go away.
Sign in to reply to this message.
Thanks, committing. https://breakpad.appspot.com/1304004/diff/120001/DEPS File DEPS (right): https://breakpad.appspot.com/1304004/diff/120001/DEPS#newcode31 DEPS:31: # in an SVN checkout of the repository. If using an SVN checkout you can still On 2014/04/02 19:52:37, Mark Mentovai wrote: > “not intended” but if you want a gclient-managed svn checkout, it should work > just fine. You should get rid of the “it is not intended” sentence and loosen > the “if using an svn checkout” one to say “if using svn directly as opposed to > via gclient sync”. Done. https://breakpad.appspot.com/1304004/diff/120001/DEPS#newcode35 DEPS:35: # Cross-platform dependencies. On 2014/04/02 19:52:37, Mark Mentovai wrote: > All dependencies are cross-platform now, this comment doesn’t disambiguate > anything and should go away. Done.
Sign in to reply to this message.
|
