Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890973005:
Fix blacklist_test_dll_3 linking every time in GN Windows. (Closed)
DescriptionFix blacklist_test_dll_3 linking every time in GN Windows.
This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date.
The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them.
Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files.
BUG=586970
Committed: https://crrev.com/c4afc4d5ba4e5202eec5486873aea0eb2b7ecac2
Cr-Commit-Position: refs/heads/master@{#387735}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : comment #Messages
Total messages: 23 (9 generated)
Description was changed from ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. ========== to ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. BUG=586970 ==========
brettw@chromium.org changed reviewers: + brucedawson@chromium.org
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890973005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890973005/20001
Definitely cleaner. Some questions - perhaps off-topic: Do you know why blacklist_test_dll_1.dll has a .def file with no export, blacklist_test_dll_2.dll has a .def file with a DummyExport, and blacklist_test_dll_1.dll has no .def file? Is that just part of what they're testing? I assume so but I'd hate to have us propagate accidental weirdness. The description needs word wrapping to fit in 72 columns. I've heard that the git standard is to not have trailing periods on the initial line of the description, although looking at the commit history shows no consistency on that point. lgtm
Maybe propagate some of the .gyp comments discussed in the bug to make the
intentions clearer? For example:
In GYP this DLL does this:
'msvs_settings': {
# There's no exports in this DLL, this tells ninja not to expect an
# import lib so that it doesn't keep rebuilding unnecessarily due to
# the .lib being "missing".
'NoImportLibrary': 'true',
},
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/15 18:51:30, brucedawson wrote: > Definitely cleaner. Some questions - perhaps off-topic: > > Do you know why blacklist_test_dll_1.dll has a .def file with no export, > blacklist_test_dll_2.dll has a .def file with a DummyExport, and > blacklist_test_dll_1.dll has no .def file? Is that just part of what they're > testing? I assume so but I'd hate to have us propagate accidental weirdness. Nope! > The description needs word wrapping to fit in 72 columns. This isn't a requirement to my knowledge and I've never done this. Some sub-projects enforce this, but Chrome doesn't
comment
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1890973005/#ps40001 (title: "comment")
I wrote a new comment that's a bit more general above the target.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890973005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890973005/40001
Message was sent while issue was closed.
Description was changed from ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. BUG=586970 ========== to ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. BUG=586970 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. BUG=586970 ========== to ========== Fix blacklist_test_dll_3 linking every time in GN Windows. This DLL would link every time you did a build because shared_libraries are declared to generate .lib files. But this shared library had no exports and no .def file which produced no .lib. The result was Ninja thought it was always out-of-date. The fix is to make the test DLLs loadable_modules which are not declared to produce a .lib file. This is also more semantically correct since one would never want to link directly to them. Clean up some of the .def file handling in the chrome_elf tests. GN now has built-in knowledge about these files. BUG=586970 Committed: https://crrev.com/c4afc4d5ba4e5202eec5486873aea0eb2b7ecac2 Cr-Commit-Position: refs/heads/master@{#387735} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c4afc4d5ba4e5202eec5486873aea0eb2b7ecac2 Cr-Commit-Position: refs/heads/master@{#387735}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Doesn't this make the test meaningless?
Message was sent while issue was closed.
On 2016/04/15 23:07:31, Nico wrote: > Doesn't this make the test meaningless? As far as I can tell, the only practical effect of this is that the .lib is no longer listed in the outputs in the ninja file. The DLLs are already listed as data deps so haven't been linked by GN. Changing to loadable_modules should have no effect on the linking od the blacklist DLLs itself or of things that depend on it via data deps. If you can think of a way this is wrong, please let me know! (It's possible the existing structure was wrong.)
Message was sent while issue was closed.
On 2016/04/15 23:22:06, brettw wrote: > On 2016/04/15 23:07:31, Nico wrote: > > Doesn't this make the test meaningless? > > As far as I can tell, the only practical effect of this is that the .lib is no > longer listed in the outputs in the ninja file. The DLLs are already listed as > data deps so haven't been linked by GN. Changing to loadable_modules should have > no effect on the linking od the blacklist DLLs itself or of things that depend > on it via data deps. If you can think of a way this is wrong, please let me > know! (It's possible the existing structure was wrong.) As long as an export-less dll is generated by this target, the test still should work (and have meaning). The sandbox module load interception has some code paths that branch depending on whether there's an export table, this test dll exercises the cases when there isn't one. |
