Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1640783002:
[device] Add setupapi.lib dependency to //device/serial (Closed)
|
Created:
4 years, 11 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 11 months ago CC:
charliea (OOO until 10-5), chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[device] Add setupapi.lib dependency to //device/serial
BUG=581702
R=thakis@chromium.org
Committed: https://crrev.com/fce23e0047286bfa3374c8d3c94e1ca7123d6bf7
Cr-Commit-Position: refs/heads/master@{#371846}
Patch Set 1 #Patch Set 2 : gyp #
Total comments: 1
Patch Set 3 : +DELAYLOAD #
Messages
Total messages: 25 (3 generated)
why does the gyp build not need this?
On 2016/01/27 at 17:13:50, thakis wrote: > why does the gyp build not need this? I don't really know. I was trying to figure that out but then I gave up because it builds and because GYP. I can take another look.
On 2016/01/27 at 17:16:11, Ken Rockot wrote: > On 2016/01/27 at 17:13:50, thakis wrote: > > why does the gyp build not need this? > > I don't really know. I was trying to figure that out but then I gave up because it builds and because GYP. I can take another look. (I suspect it has something to do with the way certain GYP dependencies are inherited)
On 2016/01/27 at 17:16:55, Ken Rockot wrote: > On 2016/01/27 at 17:16:11, Ken Rockot wrote: > > On 2016/01/27 at 17:13:50, thakis wrote: > > > why does the gyp build not need this? > > > > I don't really know. I was trying to figure that out but then I gave up because it builds and because GYP. I can take another look. > > (I suspect it has something to do with the way certain GYP dependencies are inherited) More specifically I'm guessing it's because base depends on setupapi as well, and its gyp rule has all_dependent_settings (probably incorrectly) set to link against it. I can go ahead and make the change to serial.gyp
Can you at least add it in gyp too? It sounds like it currently works there by accident.
Sure. By the way, if //base actually should expose its setupapi (et al) lib dependency publicly as it does in the gyp build, should they be linked via a public_config there as well, rather than explicitly being added to libs in the //base:base rule? On Wed, Jan 27, 2016 at 9:20 AM, <thakis@chromium.org> wrote: > Can you at least add it in gyp too? It sounds like it currently works > there by > accident. > > https://codereview.chromium.org/1640783002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hm, I think it works in gyp because base.gyp makes everything depend on setupapi.lib (see base.gyp). However base/BUILD.gn tries to do that too (see base_win_linker_flags in base/BUILD.gn) -- tools/battor depends on base in both gyp and gn. Why does the base/BUILD.gn deps stuff doesn't add this library alredy?
Ah it's just a DELAYLOAD setting, which I suspect does not imply an actual link-time dependency. Rather it's a setting to enforce if the DLL is depended upon. Which is actually all the gyp rule exposes to dependents too. I'll add an explicit -lsetupapi.lib to serial and its dependent settings. On Wed, Jan 27, 2016 at 9:23 AM, <thakis@chromium.org> wrote: > Hm, I think it works in gyp because base.gyp makes everything depend on > setupapi.lib (see base.gyp). However base/BUILD.gn tries to do that too > (see > base_win_linker_flags in base/BUILD.gn) -- tools/battor depends on base in > both > gyp and gn. Why does the base/BUILD.gn deps stuff doesn't add this library > alredy? > > https://codereview.chromium.org/1640783002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
GYP updated
Of course now this makes me wonder... do I also need to introduce a /DELAYLOAD:setupapi.dll flag for device/serial and all its dependents? *sigh* On Wed, Jan 27, 2016 at 9:35 AM, <rockot@chromium.org> wrote: > GYP updated > > https://codereview.chromium.org/1640783002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thakis@chromium.org changed reviewers: + scottmg@chromium.org
scottmg, do you know if setupapi.lib needs to be a DelayLoad? Or was that just for XP and is no longer needed? https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gyp File device/serial/serial.gyp (right): https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gy... device/serial/serial.gyp:51: '-lsetupapi.lib', I think normally .libs are passed via LinkSettings/AdditionalDependencies (https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=150)
On 2016/01/27 at 17:37:01, Ken Rockot wrote: > Of course now this makes me wonder... do I also need to introduce a > /DELAYLOAD:setupapi.dll flag for device/serial and all its dependents? > *sigh* > I mean in practice it makes no difference since base imposes that flag, on everyone, but it may still be technically undesirably to have a target introduce this dependency without also ensuring DELAYLOAD. > On Wed, Jan 27, 2016 at 9:35 AM, <rockot@chromium.org> wrote: > > > GYP updated > > > > https://codereview.chromium.org/1640783002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2016/01/27 17:40:44, Nico wrote: > scottmg, do you know if setupapi.lib needs to be a DelayLoad? Or was that just > for XP and is no longer needed? It's not needed for XP. I don't there's any reason it needs to delayload (though potentially it's required for win32k lockdown now, by accident). It was added fairly recently here https://codereview.chromium.org/644783005/ and was probably just copy-paste-induced. > > https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gyp > File device/serial/serial.gyp (right): > > https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gy... > device/serial/serial.gyp:51: '-lsetupapi.lib', > I think normally .libs are passed via LinkSettings/AdditionalDependencies > (https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=150)
On 2016/01/27 17:49:54, scottmg wrote: > On 2016/01/27 17:40:44, Nico wrote: > > scottmg, do you know if setupapi.lib needs to be a DelayLoad? Or was that just > > for XP and is no longer needed? > > It's not needed for XP. I don't there's any reason it needs to delayload (though > potentially it's required for win32k lockdown now, by accident). On second thought, I think we should keep the delayload for win32k lockdown (if the dll incorrectly does something in its WinMain, it will break renderers). But if you want to try it, you can remove it and I believe ... Something should fail. > > It was added fairly recently here https://codereview.chromium.org/644783005/ and > was probably just copy-paste-induced. > > > > > https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gyp > > File device/serial/serial.gyp (right): > > > > > https://codereview.chromium.org/1640783002/diff/20001/device/serial/serial.gy... > > device/serial/serial.gyp:51: '-lsetupapi.lib', > > I think normally .libs are passed via LinkSettings/AdditionalDependencies > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/base.gyp&l=150)
OK - I'm going to add DELAYLOAD just in case. However, maybe you can also clarify an issue for me: in gyp I see two different ways of depending on a Windows library. One is by adding "foo.lib" to msvs_settings.AdditionalDependencies, and one is by adding "-lfoo.lib" to link_settings.libraries. Are these equivalent, redundant, conflicting in any way? Is one preferred over the other?
On 2016/01/27 18:20:57, Ken Rockot wrote: > OK - I'm going to add DELAYLOAD just in case. However, maybe you can also > clarify an issue for me: in gyp I see two different ways of depending on a > Windows library. One is by adding "foo.lib" to > msvs_settings.AdditionalDependencies, and one is by adding "-lfoo.lib" to > link_settings.libraries. Are these equivalent, redundant, conflicting in any > way? Is one preferred over the other? They're equivalent and both would be redundant. link_settings is "cross-platform" (but not really at all because it has to have ".lib" and the libs are not often multi-platform anyway when they're not already a target built by gyp). AdditionalDependencies is msvs-generator specific... but the ninja generator emulates msvs, so it works there too. The -l is ugly since that's not actually how they're specified on Windows command lines. Both are widely used, if I was going to guess, I'd guess link_settings used by "linux/mac" devs, and AdditionalDependencies by "windows" devs. But who knows. Either is fine.
OK thanks for the info. I've updated the GN and GYP to also imply DELAYLOAD:setupapi.dll.
lgtm
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1640783002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1640783002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [device] Add setupapi.lib dependency to //device/serial BUG=581702 R=thakis@chromium.org ========== to ========== [device] Add setupapi.lib dependency to //device/serial BUG=581702 R=thakis@chromium.org Committed: https://crrev.com/fce23e0047286bfa3374c8d3c94e1ca7123d6bf7 Cr-Commit-Position: refs/heads/master@{#371846} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fce23e0047286bfa3374c8d3c94e1ca7123d6bf7 Cr-Commit-Position: refs/heads/master@{#371846} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
