Add support for module-definition files to gn

Add support for module-definition files to gn

BUG=

Committed: https://crrev.com/b73b2de6a23e3220c79c6bb9e9f3a7b4dd20d2dd
Cr-Commit-Position: refs/heads/master@{#332355}

[Tomasz Moniuszko, 2015-05-27 14:04:02]

tmoniuszko@opera.com changed reviewers:
+ brettw@chromium.org

[Tomasz Moniuszko, 2015-05-27 14:18:30]

Hi,

This patch set adds support for module-definition (.def) files on Windows.
"/DEF:path_to_def_file" switch and implicit dependency on .def file is added to
.ninja file. I tested it and it seems to work properly: symbols are exported and
can be found in a generated .dll library.

Could you please review this patch and let me know if this is a proper way of
providing support for .def files?

Unit tests are obviously missing. I'm also not sure if implicit_deps (with .def
file included) should be also passed to NinjaTargetWriter::WriteStampForTarget
method.

[brettw, 2015-05-27 17:27:00]

I think this is fine, we should get a test, and if you delete the preprocessor,
we can run the test on all platforms. You might also see if there's an existing
test you can add a .def file on to.

https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
tools/gn/ninja_binary_target_writer.cc:254: #if defined(OS_WIN)
I think we can run this on non-Windows as well. I'm hoping to keep us as able as
we can to generate other platforms builds for debugging purposes, and I think
this code doesn't hurt anything on other platforms. Non-Windows platforms should
just not add the .def files to the build.

Does your patch work OK if you just remove the preprocessor?

https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
tools/gn/ninja_binary_target_writer.cc:323: #if defined(OS_WIN)
Ditto. I think we can always just do this.

https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
File tools/gn/ninja_binary_target_writer.h (right):

https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
tools/gn/ninja_binary_target_writer.h:33: void WriteLinkerFlags(const
SourceFile* def_file);
Maybe call this variable optional_def_file so it's more clear that it can be
null?

[Tomasz Moniuszko, 2015-05-28 14:07:49]

On 2015/05/27 17:27:00, brettw wrote:
> I think this is fine, we should get a test, and if you delete the
preprocessor,
> we can run the test on all platforms. You might also see if there's an
existing
> test you can add a .def file on to.

I think this functionality doesn't fit any of existing NinjaBinaryTargetWriter
tests so I created new test.

> 
>
https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
> File tools/gn/ninja_binary_target_writer.cc (right):
> 
>
https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
> tools/gn/ninja_binary_target_writer.cc:254: #if defined(OS_WIN)
> I think we can run this on non-Windows as well. I'm hoping to keep us as able
as
> we can to generate other platforms builds for debugging purposes, and I think
> this code doesn't hurt anything on other platforms. Non-Windows platforms
should
> just not add the .def files to the build.
> 
> Does your patch work OK if you just remove the preprocessor?

After removing preprocessor I had to add "*.def" to sources_assignment_filter in
build/config/BUILDCONFIG.gn for non-Windows platforms to make it work correctly
on these platforms. I guess the other option would be to replace preprocessor
with 'if (settings_->IsWin())' condition but currently Settings::target_os_ is
hardcoded in Settings constructor so it won't work if non-Windows build is
generated on Windows machine.

> 
>
https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
> tools/gn/ninja_binary_target_writer.cc:323: #if defined(OS_WIN)
> Ditto. I think we can always just do this.
> 
>
https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
> File tools/gn/ninja_binary_target_writer.h (right):
> 
>
https://codereview.chromium.org/1160773002/diff/1/tools/gn/ninja_binary_targe...
> tools/gn/ninja_binary_target_writer.h:33: void WriteLinkerFlags(const
> SourceFile* def_file);
> Maybe call this variable optional_def_file so it's more clear that it can be
> null?

Done.

[Tomasz Moniuszko, 2015-06-01 09:29:35]

ping?

[brettw, 2015-06-01 19:55:19]

lgtm

[Tomasz Moniuszko, 2015-06-02 07:02:04]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2015-06-02 07:03:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160773002/20001

[commit-bot: I haz the power, 2015-06-02 08:04:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-02 08:05:11]

Patchset 2 (id:??) landed as
https://crrev.com/b73b2de6a23e3220c79c6bb9e9f3a7b4dd20d2dd
Cr-Commit-Position: refs/heads/master@{#332355}