Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132333021:
Add FreeBSD as a supported platform using ninja. (Closed)
DescriptionAdd FreeBSD as a supported platform using ninja.
TEST= run 'gclient sync' on a FreeBSD system.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250318
Patch Set 1 #
Total comments: 3
Patch Set 2 : Process comment #4 #
Total comments: 1
Patch Set 3 : Fix patch as in comment #8 #Messages
Total messages: 14 (0 generated)
On 2014/01/28 22:41:10, r.c.ladan wrote: Any news on this item?
On 2014/01/28 22:41:10, r.c.ladan wrote: Any news on this item?
Hey sorry for the delay! https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py File build/landmine_utils.py (right): https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py#newc... build/landmine_utils.py:36: return sys.platform.startswith('linux') For the purposes of this file, I think it would make more sense to just make this: ....startswith(('linux', 'freebsd'))
https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py File build/landmine_utils.py (right): https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py#newc... build/landmine_utils.py:36: return sys.platform.startswith('linux') On 2014/02/10 22:28:30, iannucci wrote: > For the purposes of this file, I think it would make more sense to just make > this: > > > ....startswith(('linux', 'freebsd')) What about platform() which cannot distinguish between Linux and FreeBSD in that case? (They are different operating systems...)
https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py File build/landmine_utils.py (right): https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py#newc... build/landmine_utils.py:36: return sys.platform.startswith('linux') On 2014/02/10 22:31:33, r.c.ladan wrote: > On 2014/02/10 22:28:30, iannucci wrote: > > For the purposes of this file, I think it would make more sense to just make > > this: > > > > > > ....startswith(('linux', 'freebsd')) > > What about platform() which cannot distinguish between Linux and FreeBSD in that > case? (They are different operating systems...) I know they're different operating systems :) The purpose of this file is to indicate when a bot needs to clobber the checkout due to a change in configuration. Since it's unlikely that we'll be introducing FreeBSD landmines, and that FreeBSD machines will want to clobber at the same times as the linux machines, it makes sense to merge the two together here. If we ever start running FreeBSD continuous integration systems, then it would make sense to split it out into a new method.
On 2014/02/10 22:38:39, iannucci wrote: > https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py > File build/landmine_utils.py (right): > > https://codereview.chromium.org/132333021/diff/1/build/landmine_utils.py#newc... > build/landmine_utils.py:36: return sys.platform.startswith('linux') > On 2014/02/10 22:31:33, r.c.ladan wrote: > > On 2014/02/10 22:28:30, iannucci wrote: > > > For the purposes of this file, I think it would make more sense to just make > > > this: > > > > > > > > > ....startswith(('linux', 'freebsd')) > > > > What about platform() which cannot distinguish between Linux and FreeBSD in > that > > case? (They are different operating systems...) > > I know they're different operating systems :) > > The purpose of this file is to indicate when a bot needs to clobber the checkout > due to a change in configuration. Since it's unlikely that we'll be introducing > FreeBSD landmines, and that FreeBSD machines will want to clobber at the same > times as the linux machines, it makes sense to merge the two together here. > > If we ever start running FreeBSD continuous integration systems, then it would > make sense to split it out into a new method. OK, I'm trying to think long-term here ;) New patch uploaded.
https://codereview.chromium.org/132333021/diff/70001/build/landmine_utils.py File build/landmine_utils.py (right): https://codereview.chromium.org/132333021/diff/70001/build/landmine_utils.py#... build/landmine_utils.py:36: return sys.platform.startswith('linux', 'freebsd') This should be a tuple though :)
On 2014/02/10 23:00:24, iannucci wrote: > https://codereview.chromium.org/132333021/diff/70001/build/landmine_utils.py > File build/landmine_utils.py (right): > > https://codereview.chromium.org/132333021/diff/70001/build/landmine_utils.py#... > build/landmine_utils.py:36: return sys.platform.startswith('linux', 'freebsd') > This should be a tuple though :) Oops, fixed.
lgtm :D You should be able to CQ this
The CQ bit was checked by r.c.ladan@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.c.ladan@gmail.com/132333021/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.c.ladan@gmail.com/132333021/120001
Message was sent while issue was closed.
Change committed as 250318 |
