|
|
Created:
7 years, 8 months ago by kenneth.r.christiansen Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDisable deprecation warnings for CUPS as the compile fails when using CUPS 1.6
This only affects the printing module.
Current Ubuntu versions ships with CUPS 1.6 which complains about
use of deprecated CUPS methods. As we need to continue supporting
platforms using older CUPS versions such as Linux and Mac OS, we
ignore the warning as an error for now.
BUG=226176
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195195
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 25 (0 generated)
It looks like you aren't listed in the AUTHORS file yet. Could you please add your name to the src/AUTHORS file? You are included in Intel's CLA.
On 2013/04/15 16:23:37, Peter Beverloo wrote: > It looks like you aren't listed in the AUTHORS file yet. Could you please add > your name to the src/AUTHORS file? You are included in Intel's CLA. Done!
I have a couple concerns: - How does anyone find out about future API deprecation in the printing module if we disable the warning? - If we do this for you, then people building on $distro may also want to add -Wfoo because building module bar emits a warning on their distro. I'd hate to go down this road. Someone asked about this previous and the answer was to just disable the warning locally: https://groups.google.com/a/chromium.org/forum/?fromgroups=#!msg/chromium-dev...
On 2013/04/15 20:24:50, Lei Zhang wrote: > I have a couple concerns: > > - How does anyone find out about future API deprecation in the printing module > if we disable the warning? That will only be an issue when we move to 1.6 or newer. We still need to support the older versions for now. When we upgrade to 1.6 or newer, we should of course remove this exception; no questions there. > - If we do this for you, then people building on $distro may also want to add > -Wfoo because building module bar emits a warning on their distro. I'd hate to > go down this road. Ubuntu is probably the most used development platform for Chromium/Linux and it makes sense to support the latest version. This is the only deprecation issue found so far.
On 2013/04/15 20:38:08, kenneth.christiansen wrote: > On 2013/04/15 20:24:50, Lei Zhang wrote: > > I have a couple concerns: > > > > - How does anyone find out about future API deprecation in the printing module > > if we disable the warning? > > That will only be an issue when we move to 1.6 or newer. We still need to > support the older versions for now. When we upgrade to 1.6 or newer, we should > of course remove this exception; no questions there. We can't just move to CUPS 1.6 since there's still plenty of users on systems with older CUPS versions. i.e. Ubuntu 12.04, Mac OS X 10.6. How about we do this instead - check the output from "cups-config --api-version" and only suppress the warning for 1.6. That way, if CUPS 1.7 comes out in the future, we would still get its new deprecation warnings. > > - If we do this for you, then people building on $distro may also want to add > > -Wfoo because building module bar emits a warning on their distro. I'd hate to > > go down this road. > > Ubuntu is probably the most used development platform for Chromium/Linux and it > makes sense to support the latest version. This is the only deprecation issue > found so far. Ubuntu 12.04 is probably the most used development platform. Most of the chromium.org build bots will run Ubuntu 12.04 for the near future. The newer versions of Ubuntu with CUPS 1.6 are short-lived releases with 9 months live spans.
> How about we do this instead - check the output from "cups-config --api-version" > and only suppress the warning for 1.6. That way, if CUPS 1.7 comes out in the > future, we would still get its new deprecation warnings. Good idea, patch uploaded!
Please fix the comment and I'll commit this for you. https://codereview.chromium.org/14262006/diff/12001/printing/printing.gyp File printing/printing.gyp (right): https://codereview.chromium.org/14262006/diff/12001/printing/printing.gyp#new... printing/printing.gyp:176: # CUPS 1.6 deprecated the PPD APIs, but we will stay with this API Can you wrap at 80 lines? We try to when possible. Also, can you mention bug 226176 in this comment?
Sure thing. Now wrapped at 80 chars and added link to the bug report.
On 2013/04/15 23:07:32, kenneth.christiansen wrote: > Sure thing. Now wrapped at 80 chars and added link to the bug report. lgtm It turns out our commit queue is closed for maintenance. I'll commit this once that reopens.
Please ignore patch set 6 and 7. I apparently don't know how to use the tools yet.
On 2013/04/16 14:15:33, kenneth.christiansen wrote: > Please ignore patch set 6 and 7. I apparently don't know how to use the tools > yet. You can delete them. Please do.
> You can delete them. Please do. Can you explain me how? I dont see this option
On 2013/04/16 18:02:53, kenneth.christiansen wrote: > > You can delete them. Please do. > > Can you explain me how? I dont see this option Oh, it might only appear for committers, but even though I have the option, I can't delete your patchsets. I guess you'll just have to re-upload patch set 5 as 8.
> Oh, it might only appear for committers, but even though I have the option, I > can't delete your patchsets. I guess you'll just have to re-upload patch set 5 > as 8. Done!
Are you landing this, or is it OK landing this manually tomorrow (I can get Beverloo to do it)?
On 2013/04/16 22:22:05, kenneth.christiansen wrote: > Are you landing this, or is it OK landing this manually tomorrow (I can get > Beverloo to do it)? It's easiest for the CQ to land it, since it'll be committed under your name rather than mine/Peter's if one of us land it manually for you. Are you in a hurry to land this? If not, we can just wait for CQ to go back online and commit it then.
On 2013/04/16 22:24:00, Lei Zhang wrote: > On 2013/04/16 22:22:05, kenneth.christiansen wrote: > > Are you landing this, or is it OK landing this manually tomorrow (I can get > > Beverloo to do it)? > > It's easiest for the CQ to land it, since it'll be committed under your name > rather than mine/Peter's if one of us land it manually for you. Are you in a > hurry to land this? If not, we can just wait for CQ to go back online and commit > it then. I have another patch pending for Chromium, but I can add my name to AUTHORS in that one instead.
On 2013/04/16 22:32:48, kenneth.christiansen wrote: > I have another patch pending for Chromium, but I can add my name to AUTHORS in > that one instead. Depending on how soon you want to land the other CL, you can do it there too and then update this CL to not have the AUTHORS file. If the other reviewers want to land that CL lands through CQ, then you are stuck on the CQ just as you are here. I'm hoping the CQ will come back online soon. :\
CQ is back today. If you want this to go in first, add back the AUTHORS file. Otherwise commit the other CL first.
On 2013/04/18 00:09:39, Lei Zhang wrote: > CQ is back today. If you want this to go in first, add back the AUTHORS file. > Otherwise commit the other CL first. Another patch was committed, which added me to AUTHORS. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14262...
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenneth.r.christiansen@intel.com/14262...
Message was sent while issue was closed.
Change committed as 195195 |