Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1570113002:
Visual Studio generators for GN (Closed)
|
Created:
4 years, 11 months ago by Tomasz Moniuszko Modified:
4 years, 10 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVisual Studio generators for GN
BUG=305761
Committed: https://crrev.com/050f20691a64eda7dc6df981bd576856fa19be4c
Cr-Commit-Position: refs/heads/master@{#372354}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Generate .sln and .vcxproj.filters files. Add tests. #
Total comments: 6
Patch Set 3 : Move XmlElementWriter to separate file + minor fixes #
Total comments: 14
Patch Set 4 : Address review issues #Patch Set 5 : Fix clang compilation issues #Patch Set 6 : Fix more compilation issues #Patch Set 7 : Fix tests failing on non-Windows platforms #
Total comments: 5
Messages
Total messages: 58 (15 generated)
tmoniuszko@opera.com changed reviewers: + brettw@chromium.org
Hi, I'm uploading initial work on supporting generators for Visual Studio files in GN. Only .vcxproj are generated so far but I plan to add also .sln files generation. I'm uploading unfinished work because I would like to know if I'm going in the right direction. What do you think? Thanks, Tomasz
Looks like a nice start. https://codereview.chromium.org/1570113002/diff/1/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/command_gen.cc#new... tools/gn/command_gen.cc:169: " gn gen [--ide=IDE] <out_dir>\n" --ide=<ide_name> (and same below) to make more clear that the parameter is not a literal. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:35: class XmlElement { I feel like this should be XMLElementOuptut or XMLElementWriter or something to make it clear that this is something that writes XML to files rather than representing some DOM node. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:82: for (auto attribute : attributes) { I haven't been using {} for single-line stuff like this in GN. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:189: // TODO(tmoniuszko): This should be taken from GN args or system environment. I think we should be able to extract these from the include_dirs for a target. I bet there's a simple pattern we can use to find them. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:221: return ""; return std::string(); instead. More semantically correct and the current one actually constructs a string with a constant which is less efficient than just zero-initializing everything. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:236: void ParseCompilerOption(const std::string& cflag, CompilerOptions* options) { Just checking that we really need to do this. I might have hoped that specifying the flags in "other options" would be sufficient for it to work. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:236: void ParseCompilerOption(const std::string& cflag, CompilerOptions* options) { You can do this in a follow-up, but I would suggest separating out the visual studio information extraction (this stuff and the include directory finding) into a separate source file, and we should also get some good unit tests for the functions there. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:319: for (const std::string& flag : cflags) { No {} https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:523: project.SubElement("Target", {{"Name", "Build"}}); This {{...}} is "uniform initialization syntax" right? I think this is currently disallowed by the style guide: https://chromium-cpp.appspot.com/ We may need to find an alternative for expressing this. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:588: std::ofstream file; In the other writers, you can see I made a stringstream and then write that to a file. It turns out that's much much faster than using ofstream directly. I would do that here. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.h (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. The current copyright guidance has no "(c)". (same in the other new file) https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.h:15: struct ProjectConfiguration; I think this is too general a name to expose in the global namespace. I'd actually make this an inner class inside VisualStudioWriter (can be private) and then move the current WriteMsBuildProject into this class (doesn't need a build_settings param then either).
bratell@opera.com changed reviewers: + bratell@opera.com
https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:418: properties->SubElement("LinkIncremental") LinkIncremental really needed? VS will not do any linking, only ninja will, I believe.
https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:588: std::ofstream file; On 2016/01/08 23:51:50, brettw wrote: > In the other writers, you can see I made a stringstream and then write that to a > file. It turns out that's much much faster than using ofstream directly. I would > do that here. Just tested. Such a change changes the generation time from ~750 ms (fastest 684 ms) to ~600 ms (fastest 587 ms) so 20% less time. Furthermore changing the code to only write the file if it has actually changed cuts the time by another half for the common scenario.
On 2016/01/14 15:41:14, Daniel Bratell wrote: > Furthermore changing the code to only write the file if it has actually changed > cuts the time by another half for the common scenario. That's a good idea, at least on Windows. I'll make a note to look into that for the Ninja writer.
On 2016/01/19 22:55:40, brettw wrote: > On 2016/01/14 15:41:14, Daniel Bratell wrote: > > Furthermore changing the code to only write the file if it has actually > changed > > cuts the time by another half for the common scenario. > > That's a good idea, at least on Windows. I'll make a note to look into that for > the Ninja writer. For VS writer it's included in Patch Set 2. Daniel Bratell is the author of that part.
https://codereview.chromium.org/1570113002/diff/1/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/command_gen.cc#new... tools/gn/command_gen.cc:169: " gn gen [--ide=IDE] <out_dir>\n" On 2016/01/08 23:51:50, brettw wrote: > --ide=<ide_name> > (and same below) to make more clear that the parameter is not a literal. Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:35: class XmlElement { On 2016/01/08 23:51:50, brettw wrote: > I feel like this should be XMLElementOuptut or XMLElementWriter or something to > make it clear that this is something that writes XML to files rather than > representing some DOM node. Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:82: for (auto attribute : attributes) { On 2016/01/08 23:51:50, brettw wrote: > I haven't been using {} for single-line stuff like this in GN. Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:189: // TODO(tmoniuszko): This should be taken from GN args or system environment. On 2016/01/08 23:51:50, brettw wrote: > I think we should be able to extract these from the include_dirs for a target. I > bet there's a simple pattern we can use to find them. It's not available in include_dirs. In GYP build it's added by generator. I think we have to do the same. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:221: return ""; On 2016/01/08 23:51:50, brettw wrote: > return std::string(); > instead. More semantically correct and the current one actually constructs a > string with a constant which is less efficient than just zero-initializing > everything. > > Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:236: void ParseCompilerOption(const std::string& cflag, CompilerOptions* options) { On 2016/01/08 23:51:50, brettw wrote: > Just checking that we really need to do this. I might have hoped that specifying > the flags in "other options" would be sufficient for it to work. It doesn't work properly because project properties are set to default values but different values are set in "Additional Options". I'm not sure what values are passed to intellisense in this case. Also I'm getting warnings while trying to compile: 1>cl : Command line warning D9025: overriding '/W1' with '/W4' 1> 1> 1>cl : Command line warning D9025: overriding '/WX-' with '/WX' 1> 1> 1>cl : Command line warning D9025: overriding '/O2' with '/Od' 1> 1> 1>cl : Command line warning D9025: overriding '/MD' with '/MDd' https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:319: for (const std::string& flag : cflags) { On 2016/01/08 23:51:50, brettw wrote: > No {} Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:418: properties->SubElement("LinkIncremental") On 2016/01/14 12:08:56, Daniel Bratell wrote: > LinkIncremental really needed? VS will not do any linking, only ninja will, I > believe. I thought it may conflict with command-line options. But it seems it doesn't. I removed it. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:523: project.SubElement("Target", {{"Name", "Build"}}); On 2016/01/08 23:51:50, brettw wrote: > This {{...}} is "uniform initialization syntax" right? I think this is currently > disallowed by the style guide: > https://chromium-cpp.appspot.com/ > > We may need to find an alternative for expressing this. Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.h (right): https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/08 23:51:50, brettw wrote: > The current copyright guidance has no "(c)". > (same in the other new file) Done. https://codereview.chromium.org/1570113002/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.h:15: struct ProjectConfiguration; On 2016/01/08 23:51:50, brettw wrote: > I think this is too general a name to expose in the global namespace. I'd > actually make this an inner class inside VisualStudioWriter (can be private) and > then move the current WriteMsBuildProject into this class (doesn't need a > build_settings param then either). Done. ProjectConfiguration members are now a part of VisualStudioWriter.
I am currently writing the generator for eclipse. The eclipse config file is also XML. It would be nice if XmlElementWriter was in its own file
On 2016/01/22 17:20:42, pkotwicz wrote: > I am currently writing the generator for eclipse. The eclipse config file is > also XML. It would be nice if XmlElementWriter was in its own file OK. I can move it. @reviewers: Do you see any other issues?
Just a cursory review. https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc... tools/gn/command_gen.cc:159: OutputString("Generating Visual Studio projects took " + I know I did this so it's not your fault, but I see now that there is a --quiet flag to gn and that flag should prevent this from being printed. Probably easy if this output is moved up one level on the call stack. https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc... tools/gn/command_gen.cc:252: if (!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kQuiet)) { base::CommandLine::ForCurrentProcess() is now stored in a local variable |command_line| so this (old) code should be changed to use that local variable. https://codereview.chromium.org/1570113002/diff/20001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/20001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:743: << std::endl; std::endl generates the wrong type of line endings (though my VS doesn't seem to care), but worse, according to http://en.cppreference.com/w/cpp/io/manip/endl it also triggers a flush() that can degrade stream performance quite a lot.
https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc... tools/gn/command_gen.cc:159: OutputString("Generating Visual Studio projects took " + On 2016/01/25 14:06:52, Daniel Bratell wrote: > I know I did this so it's not your fault, but I see now that there is a --quiet > flag to gn and that flag should prevent this from being printed. Probably easy > if this output is moved up one level on the call stack. Done. https://codereview.chromium.org/1570113002/diff/20001/tools/gn/command_gen.cc... tools/gn/command_gen.cc:252: if (!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kQuiet)) { On 2016/01/25 14:06:52, Daniel Bratell wrote: > base::CommandLine::ForCurrentProcess() is now stored in a local variable > |command_line| so this (old) code should be changed to use that local variable. Done. https://codereview.chromium.org/1570113002/diff/20001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/20001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:743: << std::endl; On 2016/01/25 14:06:52, Daniel Bratell wrote: > std::endl generates the wrong type of line endings (though my VS doesn't seem to > care), but worse, according to http://en.cppreference.com/w/cpp/io/manip/endl it > also triggers a flush() that can degrade stream performance quite a lot. VS doesn't care about line endings. Does performance issue affect string streams or only files? GN is using std::endl for ninja generators (though '\n' is also used in few places). I think we should be consistent about std::endl usage so I'm not changing this for now.
@brettw: All required Visual Studio files are generated now. I did some tests: browsing the code, IntelliSense, executing the build and debugging works for me. I consider this feature is finished and ready for review. Could you please have a look?
lgtm https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:8: #include <fstream> Delete this when you remove the ofstream stuff. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:87: std::wstring value_name = L"KitsRoot" + base::UTF8ToWide(kWindowsKitsVersion); I try to use base::string16 now since we're trying to get rid of wstring. It's OK to assume char16 == wchar_t (for L"" strings) on Windows. The reasoning is that the code is more consistent with just string16. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:147: for (int i = path->size() - 2; i >= 0; --i) { I think this will need a static_cast<int>(path...) to make Windows warnings happy. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:159: std::ifstream data_2_file; Can you use base::ReadFileToString instead of this code? If you want to keep the size comparison optimization, just do base::GetFileSize(). This should be less code and maybe even faster (avoids opening the file?). https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:274: std::ofstream vcxproj_file; Instead of ofstream here and below, can you just call: base::WriteFile(vcxproj_path_str, ...); This should be less code and I believe it will also faster (I think fstream does buffering). On a related note, I'd still really like to see a function that conditionally writes a file if the contents are the same that can be shared with function_write_file. It doesn't need to block this patch, though. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.h (right): https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.h:8: #include <ostream> Use <iosfwd> instead, I think <ostream> introduces a static initializer.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
Random comment https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.h (right): https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.h:40: const std::string& guid); When I patched your CL locally, the clang compiler complained that this struct does not have a destructor. I think it complained about a couple of other structs not having constructors and destructors
https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:8: #include <fstream> On 2016/01/28 23:24:45, brettw wrote: > Delete this when you remove the ofstream stuff. Done. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:87: std::wstring value_name = L"KitsRoot" + base::UTF8ToWide(kWindowsKitsVersion); On 2016/01/28 23:24:45, brettw wrote: > I try to use base::string16 now since we're trying to get rid of wstring. It's > OK to assume char16 == wchar_t (for L"" strings) on Windows. The reasoning is > that the code is more consistent with just string16. Done. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:147: for (int i = path->size() - 2; i >= 0; --i) { On 2016/01/28 23:24:46, brettw wrote: > I think this will need a static_cast<int>(path...) to make Windows warnings > happy. Done. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:159: std::ifstream data_2_file; On 2016/01/28 23:24:45, brettw wrote: > Can you use base::ReadFileToString instead of this code? If you want to keep the > size comparison optimization, just do base::GetFileSize(). This should be less > code and maybe even faster (avoids opening the file?). Done. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.cc:274: std::ofstream vcxproj_file; On 2016/01/28 23:24:45, brettw wrote: > Instead of ofstream here and below, can you just call: > base::WriteFile(vcxproj_path_str, ...); > This should be less code and I believe it will also faster (I think fstream does > buffering). Done. > > On a related note, I'd still really like to see a function that conditionally > writes a file if the contents are the same that can be shared with > function_write_file. It doesn't need to block this patch, though. OK. I'll do this in separate patch. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... File tools/gn/visual_studio_writer.h (right): https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.h:8: #include <ostream> On 2016/01/28 23:24:46, brettw wrote: > Use <iosfwd> instead, I think <ostream> introduces a static initializer. Done. https://codereview.chromium.org/1570113002/diff/40001/tools/gn/visual_studio_... tools/gn/visual_studio_writer.h:40: const std::string& guid); On 2016/01/29 02:10:48, pkotwicz wrote: > When I patched your CL locally, the clang compiler complained that this struct > does not have a destructor. > > I think it complained about a couple of other structs not having constructors > and destructors No warnings on Windows. I added some ctors/dtors. I hope I fixed all the cases.
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1570113002/#ps60001 (title: "Address review issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570113002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1570113002/#ps80001 (title: "Fix clang compilation issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570113002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570113002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1570113002/#ps100001 (title: "Fix more compilation issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570113002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tmoniuszko@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1570113002/#ps120001 (title: "Fix tests failing on non-Windows platforms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570113002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Visual Studio generators for GN BUG=305761 ========== to ========== Visual Studio generators for GN BUG=305761 Committed: https://crrev.com/050f20691a64eda7dc6df981bd576856fa19be4c Cr-Commit-Position: refs/heads/master@{#372354} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/050f20691a64eda7dc6df981bd576856fa19be4c Cr-Commit-Position: refs/heads/master@{#372354}
Message was sent while issue was closed.
https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:155: base::TimeTicks begin_vs_gen = base::TimeTicks::Now(); Have you considered using base::ElapsedTimer here instead?
Message was sent while issue was closed.
https://codereview.chromium.org/1570113002/diff/120001/tools/gn/xml_element_w... File tools/gn/xml_element_writer.h (right): https://codereview.chromium.org/1570113002/diff/120001/tools/gn/xml_element_w... tools/gn/xml_element_writer.h:32: class XmlElementWriter { Hum, XmlWriter [1] had no good use for you case? [1] - https://chromium.googlesource.com/chromium/src/+/master/third_party/libxml/ch...
Message was sent while issue was closed.
https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:155: base::TimeTicks begin_vs_gen = base::TimeTicks::Now(); On 2016/01/29 16:30:12, tfarina wrote: > Have you considered using base::ElapsedTimer here instead? I was not aware of base::ElapsedTimer so I used what the rest of the code use. The code just landed on master so it should be easy to fix by someone who knows what is the best tool for this.
Message was sent while issue was closed.
Thank you for doing this! One question: I just tried this and I got 2545 projects in all.sln. Visual Studio is going to have a tough time with all that many projects in a solution. Is there anything we can do to merge some of the targets? I don't know what criteria we would use: perhaps turn targets that are only used by one other target into just a folder in the vcproj?
Message was sent while issue was closed.
one more comment: I can't right click on an include path and open it.
Message was sent while issue was closed.
On 2016/01/29 22:25:50, jam wrote: > one more comment: I can't right click on an include path and open it. Opening include path with "Open Document" context-menu option works for me. I tried opening various includes from Chromium sources and also STL and Windows SDK includes. What include path are you trying to open and from what file?
Message was sent while issue was closed.
On 2016/01/29 21:39:35, jam wrote: > Thank you for doing this! > > One question: I just tried this and I got 2545 projects in all.sln. Visual > Studio is going to have a tough time with all that many projects in a solution. > Is there anything we can do to merge some of the targets? I don't know what > criteria we would use: perhaps turn targets that are only used by one other > target into just a folder in the vcproj? Merging targets is probably possible but I'm not sure if we should do this. One to one gn target to VS project relationship is quite natural. Also more projects means that they are smaller now. Solution is loading a little bit longer than in case of GYP but I see no significant difference in performance once projects are loaded and initialized. Memory usage is also comparable 1.20GB (GN) vs 1.15GB (GYP).
Message was sent while issue was closed.
https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/1570113002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:155: base::TimeTicks begin_vs_gen = base::TimeTicks::Now(); On 2016/01/29 16:30:12, tfarina wrote: > Have you considered using base::ElapsedTimer here instead? https://codereview.chromium.org/1651113002/
Message was sent while issue was closed.
https://codereview.chromium.org/1570113002/diff/120001/tools/gn/xml_element_w... File tools/gn/xml_element_writer.h (right): https://codereview.chromium.org/1570113002/diff/120001/tools/gn/xml_element_w... tools/gn/xml_element_writer.h:32: class XmlElementWriter { On 2016/01/29 16:32:18, tfarina wrote: > Hum, XmlWriter [1] had no good use for you case? > [1] - > https://chromium.googlesource.com/chromium/src/+/master/third_party/libxml/ch... Well, XmlWriter could be used for sure but its API isn't as convenient as XmlElementWriter's. For instance to write two nested elements with attributes and text it's enough to write: scoped_ptr<XmlElementWriter> locals = project.SubElement("PropertyGroup", XmlAttributes("Label", "Locals")); locals->SubElement("PlatformToolset")->Text(kToolsetVersion); It also allows to write directly to underlying output stream either by using StartContent() method or by providing attribute value writer. Accessing stream directly should be faster than copying values several times. I wrote XmlElementWriter as a simple specialized utility at the beginning. Intended to be used internally by VisualStudioWriter only. Then I was asked to move it to separate file to make it possible to reuse it for other IDE files writers.
Message was sent while issue was closed.
On 2016/02/01 11:47:06, Tomasz Moniuszko wrote: > On 2016/01/29 22:25:50, jam wrote: > > one more comment: I can't right click on an include path and open it. > > Opening include path with "Open Document" context-menu option works for me. I > tried opening various includes from Chromium sources and also STL and Windows > SDK includes. What include path are you trying to open and from what file? hmm it doesn't work for any of the cases I tried. here are my steps: gn gen out\Debug_gn_component --ide=vs I opened all.sln in out\Debug_gn_component\all.sln some files I tried: content\shell\app\shell_main.cc: i couldn't open document any of the headers there ditto for chrome\browser\chrome_content_browser_client.cc
Message was sent while issue was closed.
On 2016/02/02 00:05:09, jam wrote: > On 2016/02/01 11:47:06, Tomasz Moniuszko wrote: > > On 2016/01/29 22:25:50, jam wrote: > > > one more comment: I can't right click on an include path and open it. > > > > Opening include path with "Open Document" context-menu option works for me. I > > tried opening various includes from Chromium sources and also STL and Windows > > SDK includes. What include path are you trying to open and from what file? > > hmm it doesn't work for any of the cases I tried. here are my steps: > gn gen out\Debug_gn_component --ide=vs > I opened all.sln in out\Debug_gn_component\all.sln > > some files I tried: > content\shell\app\shell_main.cc: i couldn't open document any of the headers > there > ditto for > chrome\browser\chrome_content_browser_client.cc I did 'git clean -xdf' to make sure everything is clean and then tried your steps. It works for me. I tried to run 'gn gen' both from Git Bash and Windows command-line. I'm able to open includes both in VS2015 and VS2013. I wonder what is the difference causes problems for you. Do you use any custom "args.gn" file? Or maybe it's because of bundled Visual Studio version. I have DEPOT_TOOLS_WIN_TOOLCHAIN="0" defined and I'm using Visual Studio installed to default location and Windows 10 SDK also in default location. I don't know if it matters though.
Message was sent while issue was closed.
I'm guessing the difference is that John has the Googler way of automatically getting the SDK, while Opera folks have it installed globally in the default location. Brett On Tue, Feb 2, 2016 at 7:48 AM, <tmoniuszko@opera.com> wrote: > > On 2016/02/02 00:05:09, jam wrote: > > On 2016/02/01 11:47:06, Tomasz Moniuszko wrote: > > > On 2016/01/29 22:25:50, jam wrote: > > > > one more comment: I can't right click on an include path and open it. > > > > > > Opening include path with "Open Document" context-menu option works for me. > I > > > tried opening various includes from Chromium sources and also STL and > Windows > > > SDK includes. What include path are you trying to open and from what file? > > > > hmm it doesn't work for any of the cases I tried. here are my steps: > > gn gen out\Debug_gn_component --ide=vs > > I opened all.sln in out\Debug_gn_component\all.sln > > > > some files I tried: > > content\shell\app\shell_main.cc: i couldn't open document any of the headers > > there > > ditto for > > chrome\browser\chrome_content_browser_client.cc > > I did 'git clean -xdf' to make sure everything is clean and then tried your > steps. It works for me. I tried to run 'gn gen' both from Git Bash and Windows > command-line. I'm able to open includes both in VS2015 and VS2013. > > I wonder what is the difference causes problems for you. Do you use any custom > "args.gn" file? Or maybe it's because of bundled Visual Studio version. I have > DEPOT_TOOLS_WIN_TOOLCHAIN="0" defined and I'm using Visual Studio installed to > default location and Windows 10 SDK also in default location. I don't know if it > matters though. > https://codereview.chromium.org/1570113002/ > > -- 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.
Message was sent while issue was closed.
On 2016/02/02 17:02:50, brettw wrote: > I'm guessing the difference is that John has the Googler way of > automatically getting the SDK, while Opera folks have it installed globally > in the default location. > I wonder if it really matters. Even if GetWindowsKitsIncludeDirs() doesn't work properly for SDK obtained in Googler way, non-SDK includes shouldn't be affected.
Message was sent while issue was closed.
On 2016/02/03 08:39:15, Tomasz Moniuszko wrote: > On 2016/02/02 17:02:50, brettw wrote: > > I'm guessing the difference is that John has the Googler way of > > automatically getting the SDK, while Opera folks have it installed globally > > in the default location. > > > > I wonder if it really matters. Even if GetWindowsKitsIncludeDirs() doesn't work > properly for SDK obtained in Googler way, non-SDK includes shouldn't be > affected. the exact error message I get is --------------------------- Microsoft Visual Studio --------------------------- File 'chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h' not found in current source file's directory or in build system paths. Current source file path: 'D:\src\chrome1\src\chrome\browser' Build system path: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include;C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\atlmfc\include;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt;;;C:\Program Files (x86)\Windows Kits\8.1\Include\um;C:\Program Files (x86)\Windows Kits\8.1\Include\shared;C:\Program Files (x86)\Windows Kits\8.1\Include\winrt;; --------------------------- OK --------------------------- strangely, even if I remove "inherit values" in the properties of that file for include paths, and I just put "D:\src\chrome1\src" it still doesn't work
Message was sent while issue was closed.
On 2016/02/04 19:02:43, jam wrote: > On 2016/02/03 08:39:15, Tomasz Moniuszko wrote: > > On 2016/02/02 17:02:50, brettw wrote: > > > I'm guessing the difference is that John has the Googler way of > > > automatically getting the SDK, while Opera folks have it installed globally > > > in the default location. > > > > > > > I wonder if it really matters. Even if GetWindowsKitsIncludeDirs() doesn't > work > > properly for SDK obtained in Googler way, non-SDK includes shouldn't be > > affected. > > the exact error message I get is > > --------------------------- > Microsoft Visual Studio > --------------------------- > File 'chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h' > not found in current source file's directory or in build system paths. > > Current source file path: 'D:\src\chrome1\src\chrome\browser' > Build system path: C:\Program Files (x86)\Microsoft Visual Studio > 14.0\VC\include;C:\Program Files (x86)\Microsoft Visual Studio > 14.0\VC\atlmfc\include;C:\Program Files (x86)\Windows > Kits\10\Include\10.0.10240.0\ucrt;;;C:\Program Files (x86)\Windows > Kits\8.1\Include\um;C:\Program Files (x86)\Windows > Kits\8.1\Include\shared;C:\Program Files (x86)\Windows Kits\8.1\Include\winrt;; > --------------------------- > OK > --------------------------- > > strangely, even if I remove "inherit values" in the properties of that file for > include paths, and I just put "D:\src\chrome1\src" it still doesn't work Your build system path is definitely missing a lot of include paths. In my chrome/browser/browser.ninja I have: include_dirs = -I../.. -Igen -I../../third_party/wtl/include -I../../third_party/khronos -I../../gpu -I../../third_party/libwebp -I../../testing/gtest/include -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/WebKit -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/protobuf/vsprojects -Igen -Igen -Igen/ui/views/resources -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -I../../third_party/dom_distiller_js/dist/proto_gen -Igen/components/strings -Igen/components/strings -Igen/components/strings -I../../v8/include -I../../third_party/boringssl/src/include -I../../third_party/mesa/src/include -Igen/ui/resources -Igen/ui/resources -I../../third_party/opus/src/include -I../../third_party/re2/src -Igen/extensions -Igen/extensions -Igen/extensions -Igen -Igen/extensions/strings -I../../third_party/cacheinvalidation/overrides -I../../third_party/cacheinvalidation/src -I../../third_party/webrtc_overrides -I../../third_party/libjingle/overrides -I../../third_party/libjingle/source -I../../testing/gtest/include -I../../third_party -I../../third_party/expat/files/lib -I../../third_party/libxml/src/include -I../../third_party/libxml/win32/include -I../../third_party/zlib -Igen/components -Igen/components -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../third_party/libaddressinput/src/cpp/include -I../../third_party/libaddressinput/chromium/override -I../../third_party/libyuv -I../../third_party/libyuv/include -I../../third_party/webrtc_overrides -I../../third_party -Igen What is reflected in chrome/browser/browser.vcxproj: <AdditionalIncludeDirectories>../../../../..;../../../../../out/Debug/gen;../../../../../third_party/wtl/include;../../../../../third_party/khronos;../../../../../gpu;../../../../../third_party/libwebp;../../../../../testing/gtest/include;../../../../../skia/config;../../../../../skia/ext;../../../../../third_party/skia/include/c;../../../../../third_party/skia/include/config;../../../../../third_party/skia/include/core;../../../../../third_party/skia/include/effects;../../../../../third_party/skia/include/images;../../../../../third_party/skia/include/lazy;../../../../../third_party/skia/include/pathops;../../../../../third_party/skia/include/pdf;../../../../../third_party/skia/include/pipe;../../../../../third_party/skia/include/ports;../../../../../third_party/skia/include/utils;../../../../../third_party/skia/include/gpu;../../../../../third_party/skia/src/gpu;../../../../../third_party/WebKit;../../../../../third_party/icu/source/common;../../../../../third_party/icu/source/i18n;../../../../../third_party/npapi;../../../../../third_party/npapi/bindings;../../../../../third_party/protobuf/src;../../../../../out/Debug/gen/protoc_out;../../../../../third_party/protobuf/src;../../../../../third_party/protobuf/vsprojects;../../../../../out/Debug/gen;../../../../../out/Debug/gen;../../../../../out/Debug/gen/ui/views/resources;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../out/Debug/gen/chrome;../../../../../third_party/dom_distiller_js/dist/proto_gen;../../../../../out/Debug/gen/components/strings;../../../../../out/Debug/gen/components/strings;../../../../../out/Debug/gen/components/strings;../../../../../v8/include;../../../../../third_party/boringssl/src/include;../../../../../third_party/mesa/src/include;../../../../../out/Debug/gen/ui/resources;../../../../../out/Debug/gen/ui/resources;../../../../../third_party/opus/src/include;../../../../../third_party/re2/src;../../../../../out/Debug/gen/extensions;../../../../../out/Debug/gen/extensions;../../../../../out/Debug/gen/extensions;../../../../../out/Debug/gen;../../../../../out/Debug/gen/extensions/strings;../../../../../third_party/cacheinvalidation/overrides;../../../../../third_party/cacheinvalidation/src;../../../../../third_party/webrtc_overrides;../../../../../third_party/libjingle/overrides;../../../../../third_party/libjingle/source;../../../../../testing/gtest/include;../../../../../third_party;../../../../../third_party/expat/files/lib;../../../../../third_party/libxml/src/include;../../../../../third_party/libxml/win32/include;../../../../../third_party/zlib;../../../../../out/Debug/gen/components;../../../../../out/Debug/gen/components;../../../../../third_party/leveldatabase;../../../../../third_party/leveldatabase/src;../../../../../third_party/leveldatabase/src/include;../../../../../third_party/libaddressinput/src/cpp/include;../../../../../third_party/libaddressinput/chromium/override;../../../../../third_party/libyuv;../../../../../third_party/libyuv/include;../../../../../third_party/webrtc_overrides;../../../../../third_party;../../../../../out/Debug/gen;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\shared;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\um;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\winrt;$(VSInstallDir)\VC\atlmfc\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> I see that you have SDK 8.1 and 10 include paths mixed. GetWindowsKitsIncludeDirs() supports only Win 10 SDK so 8.1 include paths must come from different source. Anyway, it doesn't explain why all Chromium include paths are missing. Please try to compare your .ninja and .vcxproj files for given target. <AdditionalIncludeDirectories> from .vcxproj should match include_dirs from .ninja file. Something must be badly broken if they don't.
Message was sent while issue was closed.
On 2016/02/05 14:42:46, Tomasz Moniuszko wrote: > On 2016/02/04 19:02:43, jam wrote: > > On 2016/02/03 08:39:15, Tomasz Moniuszko wrote: > > > On 2016/02/02 17:02:50, brettw wrote: > > > > I'm guessing the difference is that John has the Googler way of > > > > automatically getting the SDK, while Opera folks have it installed > globally > > > > in the default location. > > > > > > > > > > I wonder if it really matters. Even if GetWindowsKitsIncludeDirs() doesn't > > work > > > properly for SDK obtained in Googler way, non-SDK includes shouldn't be > > > affected. > > > > the exact error message I get is > > > > --------------------------- > > Microsoft Visual Studio > > --------------------------- > > File > 'chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h' > > not found in current source file's directory or in build system paths. > > > > Current source file path: 'D:\src\chrome1\src\chrome\browser' > > Build system path: C:\Program Files (x86)\Microsoft Visual Studio > > 14.0\VC\include;C:\Program Files (x86)\Microsoft Visual Studio > > 14.0\VC\atlmfc\include;C:\Program Files (x86)\Windows > > Kits\10\Include\10.0.10240.0\ucrt;;;C:\Program Files (x86)\Windows > > Kits\8.1\Include\um;C:\Program Files (x86)\Windows > > Kits\8.1\Include\shared;C:\Program Files (x86)\Windows > Kits\8.1\Include\winrt;; > > --------------------------- > > OK > > --------------------------- > > > > strangely, even if I remove "inherit values" in the properties of that file > for > > include paths, and I just put "D:\src\chrome1\src" it still doesn't work > > Your build system path is definitely missing a lot of include paths. > > In my chrome/browser/browser.ninja I have: <snip to fit in 10K limit> Mine is include_dirs = -I../.. -Igen -I../../third_party/wtl/include -I../../third_party/khronos -I../../gpu -I../../third_party/libwebp -I../../testing/gtest/include -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/WebKit -I../../v8/include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/protobuf/vsprojects -Igen -Igen -Igen/ui/views/resources -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -I../../third_party/dom_distiller_js/dist/proto_gen -Igen/components/strings -Igen/components/strings -Igen/components/strings -I../../v8/include -I../../third_party/boringssl/src/include -I../../third_party/mesa/src/include -Igen/ui/resources -Igen/ui/resources -I../../third_party/opus/src/include -I../../third_party/re2/src -Igen/extensions -Igen/extensions -Igen/extensions -Igen -Igen/extensions/strings -I../../third_party/cacheinvalidation/overrides -I../../third_party/cacheinvalidation/src -I../../third_party/webrtc_overrides -I../../third_party/libjingle/overrides -I../../third_party/libjingle/source -I../../testing/gtest/include -I../../third_party -I../../third_party/expat/files/lib -I../../third_party/libxml/src/include -I../../third_party/libxml/win32/include -I../../third_party/zlib -Igen/components -Igen/components -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../third_party/libaddressinput/src/cpp/include -I../../third_party/libaddressinput/chromium/override -I../../third_party/libyuv -I../../third_party/libyuv/include -I../../third_party/webrtc_overrides -I../../third_party -Igen which looks the same as yours except that I have an extra "-I../../v8/include", probably because of different revisions that we're synced at. > > What is reflected in chrome/browser/browser.vcxproj: > > <snip to hit 10K limit> mine are the same other than the out directory name: <AdditionalIncludeDirectories>../../../../..;../../../../../out/Debug_gn_component/gen;../../../../../third_party/wtl/include;../../../../../third_party/khronos;../../../../../gpu;../../../../../third_party/libwebp;../../../../../testing/gtest/include;../../../../../skia/config;../../../../../skia/ext;../../../../../third_party/skia/include/c;../../../../../third_party/skia/include/config;../../../../../third_party/skia/include/core;../../../../../third_party/skia/include/effects;../../../../../third_party/skia/include/images;../../../../../third_party/skia/include/lazy;../../../../../third_party/skia/include/pathops;../../../../../third_party/skia/include/pdf;../../../../../third_party/skia/include/pipe;../../../../../third_party/skia/include/ports;../../../../../third_party/skia/include/utils;../../../../../third_party/skia/include/gpu;../../../../../third_party/skia/src/gpu;../../../../../third_party/WebKit;../../../../../third_party/icu/source/common;../../../../../third_party/icu/source/i18n;../../../../../third_party/npapi;../../../../../third_party/npapi/bindings;../../../../../third_party/protobuf/src;../../../../../out/Debug_gn_component/gen/protoc_out;../../../../../third_party/protobuf/src;../../../../../third_party/protobuf/vsprojects;../../../../../out/Debug_gn_component/gen;../../../../../out/Debug_gn_component/gen;../../../../../out/Debug_gn_component/gen/ui/views/resources;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../out/Debug_gn_component/gen/chrome;../../../../../third_party/dom_distiller_js/dist/proto_gen;../../../../../out/Debug_gn_component/gen/components/strings;../../../../../out/Debug_gn_component/gen/components/strings;../../../../../out/Debug_gn_component/gen/components/strings;../../../../../v8/include;../../../../../third_party/boringssl/src/include;../../../../../third_party/mesa/src/include;../../../../../out/Debug_gn_component/gen/ui/resources;../../../../../out/Debug_gn_component/gen/ui/resources;../../../../../third_party/opus/src/include;../../../../../third_party/re2/src;../../../../../out/Debug_gn_component/gen/extensions;../../../../../out/Debug_gn_component/gen/extensions;../../../../../out/Debug_gn_component/gen/extensions;../../../../../out/Debug_gn_component/gen;../../../../../out/Debug_gn_component/gen/extensions/strings;../../../../../third_party/cacheinvalidation/overrides;../../../../../third_party/cacheinvalidation/src;../../../../../third_party/webrtc_overrides;../../../../../third_party/libjingle/overrides;../../../../../third_party/libjingle/source;../../../../../testing/gtest/include;../../../../../third_party;../../../../../third_party/expat/files/lib;../../../../../third_party/libxml/src/include;../../../../../third_party/libxml/win32/include;../../../../../third_party/zlib;../../../../../out/Debug_gn_component/gen/components;../../../../../out/Debug_gn_component/gen/components;../../../../../third_party/leveldatabase;../../../../../third_party/leveldatabase/src;../../../../../third_party/leveldatabase/src/include;../../../../../third_party/libaddressinput/src/cpp/include;../../../../../third_party/libaddressinput/chromium/override;../../../../../third_party/libyuv;../../../../../third_party/libyuv/include;../../../../../third_party/webrtc_overrides;../../../../../third_party;../../../../../out/Debug_gn_component/gen; C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\shared;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\um;C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\winrt;$(VSInstallDir)\VC\atlmfc\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> > > I see that you have SDK 8.1 and 10 include paths mixed. > GetWindowsKitsIncludeDirs() supports only Win 10 SDK so 8.1 include paths must > come from different source. > > Anyway, it doesn't explain why all Chromium include paths are missing. Please > try to compare your .ninja and .vcxproj files for given target. > <AdditionalIncludeDirectories> from .vcxproj should match include_dirs from > .ninja file. Something must be badly broken if they don't.
Message was sent while issue was closed.
btw what's your args.gn file look like? I tried component and static configs, both debug and x86. neither worked. I'm using VS 2015. I tried using procmon to look at the paths that VS tries to open when I open a file using right click. If I pick a file in content/renderer, say foo.h, it tries to open src/content/renderer/content/renderer/foo.h. It doesn't use any of the other include paths that are inherited, which is perplexing.
Message was sent while issue was closed.
On 2016/02/12 17:58:38, jam wrote: > btw what's your args.gn file look like? I tried component and static configs, > both debug and x86. neither worked. > > I'm using VS 2015. > > I tried using procmon to look at the paths that VS tries to open when I open a > file using right click. If I pick a file in content/renderer, say foo.h, it > tries to open src/content/renderer/content/renderer/foo.h. It doesn't use any of > the other include paths that are inherited, which is perplexing. actually, total operator error. i had intellisense turned off in VS! fixing that made things work.
Message was sent while issue was closed.
On 2016/02/12 19:26:00, jam wrote: > On 2016/02/12 17:58:38, jam wrote: > > btw what's your args.gn file look like? I tried component and static configs, > > both debug and x86. neither worked. > > > > I'm using VS 2015. > > > > I tried using procmon to look at the paths that VS tries to open when I open a > > file using right click. If I pick a file in content/renderer, say foo.h, it > > tries to open src/content/renderer/content/renderer/foo.h. It doesn't use any > of > > the other include paths that are inherited, which is perplexing. > > actually, total operator error. i had intellisense turned off in VS! fixing that > made things work. Good to hear it's working :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
