Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213363002:
Don't upload correct project name anymore. (Closed)
|
Created:
5 years, 5 months ago by Adrian Kuegel Modified:
5 years, 2 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/remotes/origin/master Project:
depot_tools Visibility:
Public. |
DescriptionDon't upload correct project name anymore.
All CQ supported projects already have git mirrors. We don't
want to support svn checkouts anymore.
BUG=503847
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295882
Patch Set 1 #Patch Set 2 : Replace project name by a name indicating the user should use git instead of svn. #Patch Set 3 : Remove unnecessary comment. #Messages
Total messages: 25 (4 generated)
akuegel@chromium.org changed reviewers: + phajdan.jr@chromium.org
Pawel, can you please review this CL?
phajdan.jr@chromium.org changed reviewers: + maruel@chromium.org
LGTM, thanks for the fix. +maruel Note that in general I don't consider gcl supported. People should be advised not to use it. Consider adding such a message to gcl.
On 2015/06/30 09:09:57, Paweł Hajdan Jr. wrote: > LGTM, thanks for the fix. > > +maruel > > Note that in general I don't consider gcl supported. People should be advised > not to use it. Consider adding such a message to gcl. Yes, I am aware that we want to move away from gcl, and I commented accordingly in the bug. However I think it will still take some time, and it seems some people are still using it, and the fix in this case is easy.
On 2015/06/30 09:15:21, Adrian Kuegel wrote: > On 2015/06/30 09:09:57, Paweł Hajdan Jr. wrote: > > LGTM, thanks for the fix. > > > > +maruel > > > > Note that in general I don't consider gcl supported. People should be advised > > not to use it. Consider adding such a message to gcl. > > Yes, I am aware that we want to move away from gcl, and I commented accordingly > in the bug. However I think it will still take some time, and it seems some > people are still using it, and the fix in this case is easy. gcl IS going away. In particular in these cases the user should use the git svn checkout. The warning message is there for a reason. AFAIK we do not have any project that do not have a git svn checkout that is also supported by the CQ, so it'd be better to set a flag in a way that inhibits the CQ instead of patching this up.
On 2015/06/30 14:09:42, M-A Ruel wrote: > On 2015/06/30 09:15:21, Adrian Kuegel wrote: > > On 2015/06/30 09:09:57, Paweł Hajdan Jr. wrote: > > > LGTM, thanks for the fix. > > > > > > +maruel > > > > > > Note that in general I don't consider gcl supported. People should be > advised > > > not to use it. Consider adding such a message to gcl. > > > > Yes, I am aware that we want to move away from gcl, and I commented > accordingly > > in the bug. However I think it will still take some time, and it seems some > > people are still using it, and the fix in this case is easy. > > gcl IS going away. In particular in these cases the user should use the git svn > checkout. The warning message is there for a reason. > > AFAIK we do not have any project that do not have a git svn checkout that is > also supported by the CQ, so it'd be better to set a flag in a way that inhibits > the CQ instead of patching this up. Not sure I understand what you mean with "set a flag". CQ will not even notice a CL that does not have the project field, so the user would check the commit box and wonder why nothing is happening. And we already have a warning that gcl is going away, so not sure why we should do anything in addition to that. I also warned Nico in the bug.
On 2015/06/30 14:17:11, Adrian Kuegel wrote: > On 2015/06/30 14:09:42, M-A Ruel wrote: > > On 2015/06/30 09:15:21, Adrian Kuegel wrote: > > > On 2015/06/30 09:09:57, Paweł Hajdan Jr. wrote: > > > > LGTM, thanks for the fix. > > > > > > > > +maruel > > > > > > > > Note that in general I don't consider gcl supported. People should be > > advised > > > > not to use it. Consider adding such a message to gcl. > > > > > > Yes, I am aware that we want to move away from gcl, and I commented > > accordingly > > > in the bug. However I think it will still take some time, and it seems some > > > people are still using it, and the fix in this case is easy. > > > > gcl IS going away. In particular in these cases the user should use the git > svn > > checkout. The warning message is there for a reason. > > > > AFAIK we do not have any project that do not have a git svn checkout that is > > also supported by the CQ, so it'd be better to set a flag in a way that > inhibits > > the CQ instead of patching this up. > > Not sure I understand what you mean with "set a flag". CQ will not even notice a > CL that does not have the project field, so the user would check the commit box > and wonder why nothing is happening. And we already have a warning that gcl is > going away, so not sure why we should do anything in addition to that. I also > warned Nico in the bug. What I'm proposing is to set an invalid project that the CQ will notice and will tell the user to upgrade to git.
On 2015/06/30 14:19:46, M-A Ruel wrote: > On 2015/06/30 14:17:11, Adrian Kuegel wrote: > > On 2015/06/30 14:09:42, M-A Ruel wrote: > > > On 2015/06/30 09:15:21, Adrian Kuegel wrote: > > > > On 2015/06/30 09:09:57, Paweł Hajdan Jr. wrote: > > > > > LGTM, thanks for the fix. > > > > > > > > > > +maruel > > > > > > > > > > Note that in general I don't consider gcl supported. People should be > > > advised > > > > > not to use it. Consider adding such a message to gcl. > > > > > > > > Yes, I am aware that we want to move away from gcl, and I commented > > > accordingly > > > > in the bug. However I think it will still take some time, and it seems > some > > > > people are still using it, and the fix in this case is easy. > > > > > > gcl IS going away. In particular in these cases the user should use the git > > svn > > > checkout. The warning message is there for a reason. > > > > > > AFAIK we do not have any project that do not have a git svn checkout that is > > > also supported by the CQ, so it'd be better to set a flag in a way that > > inhibits > > > the CQ instead of patching this up. > > > > Not sure I understand what you mean with "set a flag". CQ will not even notice > a > > CL that does not have the project field, so the user would check the commit > box > > and wonder why nothing is happening. And we already have a warning that gcl is > > going away, so not sure why we should do anything in addition to that. I also > > warned Nico in the bug. > > What I'm proposing is to set an invalid project that the CQ will notice and will > tell the user to upgrade to git. I don't want to invest too much time on this. This fix would be easy, the other thing requires two CLs. If people are still using svn checkout despite the warnings which are already printed, I think that if they see the CQ warning, they may just land the CL manually. @Pawel: what do you think about this CQ warning?
On 2015/06/30 14:26:12, Adrian Kuegel wrote:
> I don't want to invest too much time on this. This fix would be easy, the
other
> thing requires two CLs. If people are still using svn checkout despite the
> warnings which are already printed, I think that if they see the CQ warning,
> they may just land the CL manually.
>
> @Pawel: what do you think about this CQ warning?
what happens if you forcibly add
upload_arg.append("--project=SWITCH_TO_GIT_ALREADY")
I prefer to break users than let this lingering.
On 2015/06/30 14:30:19, M-A Ruel wrote:
> On 2015/06/30 14:26:12, Adrian Kuegel wrote:
> > I don't want to invest too much time on this. This fix would be easy, the
> other
> > thing requires two CLs. If people are still using svn checkout despite the
> > warnings which are already printed, I think that if they see the CQ warning,
> > they may just land the CL manually.
> >
> > @Pawel: what do you think about this CQ warning?
>
> what happens if you forcibly add
> upload_arg.append("--project=SWITCH_TO_GIT_ALREADY")
>
> I prefer to break users than let this lingering.
Ok, that sounds like an easy solution, I am going to do this now.
Ok, done. Can you please take another look?
LGTM
lgtm
The CQ bit was checked by akuegel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1213363002/#ps40001 (title: "Remove unnecessary comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213363002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=295882
Message was sent while issue was closed.
On 2015/06/30 14:50:06, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > http://src.chromium.org/viewvc/chrome?view=rev&revision=295882 Maruel, as long as the master is svn I think it's reasonable to use an svn checkout. If gcl wrong work with the cq, I'll likely just svn commit directly, so I think it's better to let gcl push the right project ID. It's not much cod.
Message was sent while issue was closed.
On 2015/06/30 15:55:10, Nico wrote: > On 2015/06/30 14:50:06, commit-bot: I haz the power wrote: > > Committed patchset #3 (id:40001) as > > http://src.chromium.org/viewvc/chrome?view=rev&revision=295882 > > Maruel, as long as the master is svn I think it's reasonable to use an svn > checkout. If gcl wrong work with the cq, I'll likely just svn commit directly, > so I think it's better to let gcl push the right project ID. It's not much cod. I'm personally getting rid of svn checkouts this summer. This excludes changing the source of truth, which will be done after. So please switch to a git checkout and move on.
Message was sent while issue was closed.
On 2015/06/30 16:05:59, M-A Ruel (OOO Wed Holiday) wrote: > On 2015/06/30 15:55:10, Nico wrote: > > On 2015/06/30 14:50:06, commit-bot: I haz the power wrote: > > > Committed patchset #3 (id:40001) as > > > http://src.chromium.org/viewvc/chrome?view=rev&revision=295882 > > > > Maruel, as long as the master is svn I think it's reasonable to use an svn > > checkout. If gcl wrong work with the cq, I'll likely just svn commit directly, > > so I think it's better to let gcl push the right project ID. It's not much > cod. > > I'm personally getting rid of svn checkouts this summer. This excludes changing > the source of truth, which will be done after. So please switch to a git > checkout and move on. That's cool. Until then, please don't block simple CLs that enable people to get work done. https://codereview.chromium.org/1220783003 now says "SWITCH_TO_GIT_ALREADY" as project. That's gotta be more code than doing the right thing.
Message was sent while issue was closed.
On 2015/07/02 06:03:28, Nico wrote: > On 2015/06/30 16:05:59, M-A Ruel (OOO Wed Holiday) wrote: > > On 2015/06/30 15:55:10, Nico wrote: > > > On 2015/06/30 14:50:06, commit-bot: I haz the power wrote: > > > > Committed patchset #3 (id:40001) as > > > > http://src.chromium.org/viewvc/chrome?view=rev&revision=295882 > > > > > > Maruel, as long as the master is svn I think it's reasonable to use an svn > > > checkout. If gcl wrong work with the cq, I'll likely just svn commit > directly, > > > so I think it's better to let gcl push the right project ID. It's not much > > cod. > > > > I'm personally getting rid of svn checkouts this summer. This excludes > changing > > the source of truth, which will be done after. So please switch to a git > > checkout and move on. > > That's cool. Until then, please don't block simple CLs that enable people to get > work done. https://codereview.chromium.org/1220783003 now says > "SWITCH_TO_GIT_ALREADY" as project. That's gotta be more code than doing the > right thing. (fixed now in patch set 2, since I locally patched in patch set 1 of this review here – thanks akuegel. It is indeed shorter.)
Message was sent while issue was closed.
On 2015/07/02 06:05:59, Nico wrote: > On 2015/07/02 06:03:28, Nico wrote: > > On 2015/06/30 16:05:59, M-A Ruel (OOO Wed Holiday) wrote: > > > On 2015/06/30 15:55:10, Nico wrote: > > > > On 2015/06/30 14:50:06, commit-bot: I haz the power wrote: > > > > > Committed patchset #3 (id:40001) as > > > > > http://src.chromium.org/viewvc/chrome?view=rev&revision=295882 > > > > > > > > Maruel, as long as the master is svn I think it's reasonable to use an svn > > > > checkout. If gcl wrong work with the cq, I'll likely just svn commit > > directly, > > > > so I think it's better to let gcl push the right project ID. It's not much > > > cod. > > > > > > I'm personally getting rid of svn checkouts this summer. This excludes > > changing > > > the source of truth, which will be done after. So please switch to a git > > > checkout and move on. > > > > That's cool. Until then, please don't block simple CLs that enable people to > get > > work done. https://codereview.chromium.org/1220783003 now says > > "SWITCH_TO_GIT_ALREADY" as project. That's gotta be more code than doing the > > right thing. > > (fixed now in patch set 2, since I locally patched in patch set 1 of this review > here – thanks akuegel. It is indeed shorter.) Please switch to a git checkout already.
Message was sent while issue was closed.
(this cost me another 10 min today since i was on some old machine with an old checkout and needed to do something. it was pretty frustrating having to hunt down this cl, given that the change to make this work for real is less code than the current passive-aggressive version. please reconsider; the change has cost me a bunch of time already)
Message was sent while issue was closed.
On 2015/09/28 18:58:29, Nico (offline until Fri Oct 9) wrote: > (this cost me another 10 min today since i was on some old machine with an old > checkout and needed to do something. it was pretty frustrating having to hunt > down this cl, given that the change to make this work for real is less code than > the current passive-aggressive version. please reconsider; the change has cost > me a bunch of time already) I agree it's quite passive-aggressive. That said, I'm sad to hear that you still have svn checkouts and that you had trouble with git-svn in general. I'm fine to get a still-somewhat-passive-aggressive but still functional CL instead. |
