|
|
Created:
10 years, 1 month ago by Craig Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux: use internal copy of libjpeg by default with mangled symbol names.
Users who want to explicitely link against the system libjpeg can just
use GYP_DEFINES=use_system_libjpeg=1 when building.
ChromeOS is still built against the system libjpeg.
BUG=30288, 31427
TEST=compiles, trybots
Patch Set 1 #Patch Set 2 : leave chromeos alone #
Messages
Total messages: 18 (0 generated)
This should fix the mixed libjpeg issue at the expense of using our own internal libjpeg and consequently bloating up the binary. The symbols probably don't need to be mangled for the normal build due to visibility=hidden but it may matter for the shared build so I've made sure to hide everything libjpeg might export. Please review. Thank you.
+hbono who seems to be doing libjpeg-turbo stuff ( crbug.com/48789 ) hbono: are you planning on nuking the existing libjpeg stuff any time soon or will we continue to have both options available?
ping. Any comments?
sorry for the lack of response. I'm more comfortable letting Evan Martin review this one
I'm not clear on why we want to do this. I think we don't want to support Google Chrome on distros other than the list we test on... ?
On 2010/12/10 21:16:44, Evan Martin wrote: > I'm not clear on why we want to do this. I think we don't want to support > Google Chrome on distros other than the list we test on... ? It's broken on opensuse 11.3 and that is on the supported list of distros AFAIK. Plus I'm just doing what issue 31427 suggests.
Lei, what do you think? I reluctantly have come to think this is a good idea. PS: now that we hide all our symbols, I don't think we need to mangle the names anymore. But that can be a separate change.
On 2010/12/13 19:12:46, Evan Martin wrote: > Lei, what do you think? I reluctantly have come to think this is a good idea. > > PS: now that we hide all our symbols, I don't think we need to mangle the names > anymore. But that can be a separate change. I'm ok with this, but I wonder if switching to libjpeg-turbo will fix this problem all together.
On 2010/12/13 20:48:18, Lei Zhang wrote: > I'm ok with this, but I wonder if switching to libjpeg-turbo will > fix this problem all together. Unfortunately, it is not so easy to build libjpeg-turbo on Linux because of GYP Issue 102: <http://code.google.com/p/gyp/issues/detail?id=102>. (I'm not sure why make.py filters out object files from a sources list even though xcode.py and msvc.py allow us to include object files in a sources list.) Regards, Hironori Bono
So should we wait for gyp issue 102 to be fixed and then use libjpeg_turbo?
On 2010/12/21 15:10:23, Craig wrote: > So should we wait for gyp issue 102 to be fixed and then use libjpeg_turbo? In the interest of making progress on this, I fixed issue 102.
On 2010/12/21 18:12:19, Evan Martin wrote: > On 2010/12/21 15:10:23, Craig wrote: > > So should we wait for gyp issue 102 to be fixed and then use libjpeg_turbo? > > In the interest of making progress on this, I fixed issue 102. Super, thank you! I see the issue 102 fix has landed in gyp. How are we supposed to try changes to libjpeg-turbo? For example if I wanted to modify the libjpeg_turbo gyp file to remove the issue 102 workaround and roll chromium DEPS to pick up a new gyp revision etc. ( I could make a patch that adds the entire modified libjpeg-turbo in a separate dir and point various things at that purely to get some test coverage but it seems incredibly messy and maybe there's an easier way )
On 2011/01/10 16:19:01, Craig wrote: > > In the interest of making progress on this, I fixed issue 102. > > Super, thank you! I see the issue 102 fix has landed in gyp. > > How are we supposed to try changes to libjpeg-turbo? For example if I wanted to > modify the libjpeg_turbo gyp file to remove the issue 102 workaround and roll > chromium DEPS to pick up a new gyp revision etc. I'm not sure I follow you -- how is this different than any other change? I would suggest changing DEPS separately first (to verify nothing else breaks). And use the trybots. :) > ( I could make a patch that adds the entire modified libjpeg-turbo in a > separate dir and point various things at that purely to get some test coverage > but it seems incredibly messy and maybe there's an easier way ) Agreed. If for some reason you need to modify libjpeg_turbo and gyp simultaneously, then you can do the DEPS change at the same time, that is ok too
On 2011/01/10 17:15:49, Evan Martin wrote: > On 2011/01/10 16:19:01, Craig wrote: > > > In the interest of making progress on this, I fixed issue 102. > > > > Super, thank you! I see the issue 102 fix has landed in gyp. > > > > How are we supposed to try changes to libjpeg-turbo? For example if I wanted > to > > modify the libjpeg_turbo gyp file to remove the issue 102 workaround and roll > > chromium DEPS to pick up a new gyp revision etc. > > I'm not sure I follow you -- how is this different than any other change? The DEPS roll to pick up issue 102's fix is not an issue, it's the libjpeg_turbo change that's tricky to test properly - third_party/libjpeg_turbo/libjpeg.gyp is pulled via svn using DEPS so conventionally I'd need to already have committed a libjpeg_turbo/libjpeg.gyp change (which I can only test locally in linux) in order to be able to test it on other platforms using the try-servers by rolling DEPS. Anyway, I'll cook up something that lets me test the change in one monster throw-away chunk including a modified libjpeg_turbo to ensure the libjpeg.gyp change is good before submitting the individual bits for review.
On 2011/01/10 18:24:36, Craig wrote: > Anyway, I'll cook up something that lets me test the change in one monster > throw-away chunk including a modified libjpeg_turbo to ensure the libjpeg.gyp > change is good before submitting the individual bits for review. Ah, I see. So you need to make a libjpeg change, and then change DEPS for gyp and libjpeg simultaneously? I think it's fine to make the jpeg change assuming that the gyp change is correct.
On 2011/01/10 18:29:53, Evan Martin wrote: > On 2011/01/10 18:24:36, Craig wrote: > > Anyway, I'll cook up something that lets me test the change in one monster > > throw-away chunk including a modified libjpeg_turbo to ensure the libjpeg.gyp > > change is good before submitting the individual bits for review. > > Ah, I see. So you need to make a libjpeg change, and then change DEPS for gyp > and libjpeg simultaneously? I think it's fine to make the jpeg change assuming > that the gyp change is correct. I could land the gyp DEPS change separately too but the real motivation here was to be able to test a patch that touched several "separate" repos without having to land those separate bits "upstream" first. I think I've figured out to trick the tryservers into doing what I want: # local uncomitted changes to turbo's libjpeg.gyp svn diff third_party/libjpeg_turbo > diff.txt # plus gyp DEPS roll and common.gypi change to enable libjpeg git diff | cleanup_diff_somehow >> diff.txt trychange.py ... --diff diff.txt Anyway, I don't want to waste everyone's time with this stuff ... I can make it work :) I'll send out the turbo changes for review later and close this...
Greetings Craig, Thank you for your work. If it is not so easy for you to create a change for libjpeg-turbo, I'm happy to merge this change to it. Regards, Hironori Bono
hbono has fixed all the necessary bits for libjpeg_turbo (thanks!) and libjpeg_turbo will probably be enabled shortly, so I'm closing this. |