| Summary: | Unified builds broke ycm autocompletion using compilation databases | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Normal | CC: | achristensen, agomez, annulen, cadubentzen, clopez, ews-watchlist, keith_miller, lforschler, ltilve, mcatanzaro, ysuzuki | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Sergio Villar Senin
2018-02-12 02:46:56 PST
How did YCM work with headers in the past? Presumably, YCM just needs to be told that the actual source files are the unified-sources and everything else is a "header". (In reply to Keith Miller from comment #1) > How did YCM work with headers in the past? YCM allows users to specify a control file where to add some more information or provide additional behaviours. There is one in Tools/gtk/ycm_extra_conf.py which is the one I was using. As you can see when asking for the compilation flags for a header file, the ones for the corresponding .cpp are returned instead > Presumably, YCM just needs to be told that the actual source files are the > unified-sources and everything else is a "header". The problem is that YCM has a very specific interface. When you open a file in the editor (note that is never an unified file but the actual source) it asks for the compilation flags of that specific file (which is never found as the flags are used for the unified sources). The solution I foresee is one that allows ycm to map from source files to unified sources. Is that possible? Keith would it be easy to generate a key value file (or anything easily readable by python) in the build directory which specifies the unified source where each source file is included? That would allow us to fix YCM quite easily as we'd just need to return the flags of the unified source when asked for the flags of a particular source file. (In reply to Sergio Villar Senin from comment #3) > Keith would it be easy to generate a key value file (or anything easily > readable by python) in the build directory which specifies the unified > source where each source file is included? Yeah, it would be trivial to add to the generate-unified-sources.rb script. Created attachment 339238 [details]
[WIP] Patch
I followed the proposed strategy and YCM is now working here :) A few points: - I used YAML as file format for the mapping because then adding new entries is as simple as just appending new lines in the end. However YAML is not parsed by python std library like json so I used a lib. Writing to JSON would require reading the file, parsing and adding the new entries to a dict before writing again, which would be slow (I think). Unless we store the entries all in memory and only write to the file once at the end. - Maybe add a new option in generate-unified-sources.rb to avoid re-running the map writing? If so we would have to call with this option in the CMake side (just like we call with --print-bundled-sources) - I reckon some renaming on the functions/variables could be appropriate. Suggestions? Also, the YAML file has only the basename for the original source because it comes as a relative path and I didn't find a way to use turn it easily into absolute in the ruby side. This could break the parsing when there are sources with the same name in different directories. Comment on attachment 339238 [details] [WIP] Patch Attachment 339238 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7530868 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html Created attachment 339252 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
ycm is a nice-to-have-thing that's why I don't think it should force us to add another dependency. What about a key,value file? (In reply to Sergio Villar Senin from comment #10) > ycm is a nice-to-have-thing that's why I don't think it should force us to > add another dependency. What about a key,value file? I agree with you on that. I'll just save key,values to a csv file then, which is easily parsable in Python and does not require external dependencies. I should have done that from the start :) Created attachment 339382 [details]
Patch
BTW have you measured how this affects build times? We're adding additional writes so there might be some impact... (In reply to Sergio Villar Senin from comment #13) > BTW have you measured how this affects build times? We're adding additional > writes so there might be some impact... The files are written during project generation (cmake command). I measured in my machine the time to execute 'build-webkit --release --gtk --generate-project-only' five times after a clean-up: - Before the patch: 7,15s user 1,75s system 72% cpu 12,271 total 7,27s user 2,00s system 74% cpu 12,458 total 7,36s user 1,85s system 73% cpu 12,504 total 7,38s user 1,93s system 74% cpu 12,559 total 7,25s user 1,82s system 73% cpu 12,363 total -------------------------------------------- Average: 7,28s user 1,87s system 73% cpu 12,431 total - After the patch: 7,20s user 1,86s system 72% cpu 12,444 total 7,49s user 1,84s system 74% cpu 12,550 total 7,46s user 1,97s system 74% cpu 12,712 total 7,24s user 1,92s system 73% cpu 12,427 total 7,55s user 1,88s system 73% cpu 12,761 total -------------------------------------------- Average: 7,38s user 1,89s system 73% cpu 12,579 total The impact seems minimal. In case it isn't desirable to have those files written in every cmake build, we could make an option and enable it only when building with the 'build-webkit' script. > The impact seems minimal. In case it isn't desirable to have those files
> written in every cmake build, we could make an option and enable it only
> when building with the 'build-webkit' script.
Or just check for CMAKE_EXPORT_COMPILE_COMMANDS in WEBKIT_COMPUTE_SOURCES (check WebKitMacros.cmake) for passing a new option to generate-unified-sources.rb (--write-key-value-files ?)
(In reply to Carlos Eduardo Ramalho from comment #15) > > The impact seems minimal. In case it isn't desirable to have those files > > written in every cmake build, we could make an option and enable it only > > when building with the 'build-webkit' script. > > Or just check for CMAKE_EXPORT_COMPILE_COMMANDS in WEBKIT_COMPUTE_SOURCES > (check WebKitMacros.cmake) for passing a new option to > generate-unified-sources.rb (--write-key-value-files ?) I am not an expert of cmake stuff at all, but indeed this should be enabled only for developer builds. Recently I switched my vim completion to YCM and this issue is suddenly top-ranked in my priority queue :) I’m planning to extend Carlos’ great work by adding DEVELOPER_MODE care (suggested by Sergio). (In reply to Yusuke Suzuki from comment #17) > Recently I switched my vim completion to YCM and this issue is suddenly > top-ranked in my priority queue :) > I’m planning to extend Carlos’ great work by adding DEVELOPER_MODE care > (suggested by Sergio). Hi Yusuke! Another work I started was to create a convenience script to generate a "expanded" version of the compile_commands.json, replacing the UnifiedSourcesXYZ.cpp entries by the original sources. See https://lists.webkit.org/pipermail/webkit-dev/2018-March/029924.html for reference. I never tried upstreaming it though. The advantage of doing this is not being exactly tied to YCM, but supporting it too. If you are interested I could submit it as follow up patch after this one. BTW, is my patch here still working out for you? It's quite old. (In reply to Carlos Eduardo Ramalho from comment #18) > (In reply to Yusuke Suzuki from comment #17) > > Recently I switched my vim completion to YCM and this issue is suddenly > > top-ranked in my priority queue :) > > I’m planning to extend Carlos’ great work by adding DEVELOPER_MODE care > > (suggested by Sergio). > > Hi Yusuke! > > Another work I started was to create a convenience script to generate a > "expanded" version of the compile_commands.json, replacing the > UnifiedSourcesXYZ.cpp entries by the original sources. > > See https://lists.webkit.org/pipermail/webkit-dev/2018-March/029924.html for > reference. > > I never tried upstreaming it though. > > The advantage of doing this is not being exactly tied to YCM, but supporting > it too. > > If you are interested I could submit it as follow up patch after this one. > > BTW, is my patch here still working out for you? It's quite old. Interesting! Now I think the good way to fix this issue is adding an option disabling unified builds (which makes compile_commands.json expected one). (In reply to Yusuke Suzuki from comment #19) > Interesting! Now I think the good way to fix this issue is adding an option > disabling unified builds (which makes compile_commands.json expected one). Hmm indeed! This might really be the way to go. Also, to get the compile_commands.json file one does not need to compile the project, but only "generate" or "configure" it with CMake e.g. with `build-webkit --wpe --release --generate-project-only` If the option to disable Unified Sources were added, I figure someone could simply run e.g. `build-webkit --wpe --release --generate-project-only --disable-unified-sources` and copy the compile_commands.json file to project root to enable autocompletion. I think it should be best because then one could reconfigure the project with unified sources and get the 2x compilation speedup still. (In reply to Carlos Eduardo Ramalho from comment #20) > (In reply to Yusuke Suzuki from comment #19) > > Interesting! Now I think the good way to fix this issue is adding an option > > disabling unified builds (which makes compile_commands.json expected one). > > Hmm indeed! This might really be the way to go. Also, to get the > compile_commands.json file one does not need to compile the project, but > only "generate" or "configure" it with CMake e.g. with Correction: actually we still need to build the project in WebKit because we need to resolve the ForwardingHeaders. For developing, disabling unified sources might be OK because usually what will matter is sequential builds times and it should be improved by disabling unified sources actually. (On a side note, sequential builds will be faster even with unified sources after bug 192391 lands.) I was able to disable unified sources and should upload a patch soon with the option. (In reply to Carlos Eduardo Ramalho from comment #21) > (In reply to Carlos Eduardo Ramalho from comment #20) > > (In reply to Yusuke Suzuki from comment #19) > > > Interesting! Now I think the good way to fix this issue is adding an option > > > disabling unified builds (which makes compile_commands.json expected one). > > > > Hmm indeed! This might really be the way to go. Also, to get the > > compile_commands.json file one does not need to compile the project, but > > only "generate" or "configure" it with CMake e.g. with > > Correction: actually we still need to build the project in WebKit because we > need to resolve the ForwardingHeaders. Right. > > For developing, disabling unified sources might be OK because usually what > will matter is sequential builds times and it should be improved by > disabling unified sources actually. No. It significantly worse the build time. I think unified builds should be enabled by default. And we should have the option disabling it. I uploaded the patch for that option. https://bugs.webkit.org/show_bug.cgi?id=193045 > > (On a side note, sequential builds will be faster even with unified sources > after bug 192391 lands.) > > I was able to disable unified sources and should upload a patch soon with > the option. (In reply to Yusuke Suzuki from comment #22) > No. It significantly worse the build time. I think unified builds should be > enabled by default. > And we should have the option disabling it. > I uploaded the patch for that option. > https://bugs.webkit.org/show_bug.cgi?id=193045 I think I was not clear. I meant rebuild time during development, when for example only one source file needs to recompiled or a new one was added. Also I didn't mean to remove Unified Sources completely, but enable it with an option as you suggested. (In reply to Carlos Eduardo Ramalho from comment #23) > (In reply to Yusuke Suzuki from comment #22) > > No. It significantly worse the build time. I think unified builds should be > > enabled by default. > > And we should have the option disabling it. > > I uploaded the patch for that option. > > https://bugs.webkit.org/show_bug.cgi?id=193045 > > I think I was not clear. I meant rebuild time during development, when for > example only one source file needs to recompiled or a new one was added. > > Also I didn't mean to remove Unified Sources completely, but enable it with > an option as you suggested. OK, make sense. Thank you for your clarification :D (In reply to Carlos Eduardo Ramalho from comment #20) > (In reply to Yusuke Suzuki from comment #19) > > Interesting! Now I think the good way to fix this issue is adding an option > > disabling unified builds (which makes compile_commands.json expected one). > > Hmm indeed! This might really be the way to go. Also, to get the > compile_commands.json file one does not need to compile the project, but > only "generate" or "configure" it with CMake e.g. with > > `build-webkit --wpe --release --generate-project-only` > > If the option to disable Unified Sources were added, I figure someone could > simply run e.g. > > `build-webkit --wpe --release --generate-project-only > --disable-unified-sources` > > and copy the compile_commands.json file to project root to enable > autocompletion. I think it should be best because then one could reconfigure > the project with unified sources and get the 2x compilation speedup still. Yeah, do you know the way only generating compile_commands.json? If we can do that, hacky (but It Works (TM)) way can be usable! 1. First, generate JSON w/o unified builds 2. Build w/ unified builds (generates appropriate headers etc.) 3. Happy! (In reply to Yusuke Suzuki from comment #25) > > Yeah, do you know the way only generating compile_commands.json? > If we can do that, hacky (but It Works (TM)) way can be usable! > > 1. First, generate JSON w/o unified builds > 2. Build w/ unified builds (generates appropriate headers etc.) > 3. Happy! At first I thought of doing it manually, but it is counter-productive because it won't get eventual new flags or new sources files and one would have to generate the project by hand. Two hack-ish ways I can think of acomplishing it: - add a new script that would generate the project - but not build -, get the compile_commands.json and delete/move the build folder and then actually build it after configuring with unified sources. It would add another script though :( or could be an option passed to build-webkit script. - (way more intrusive) add cmake library targets, excluded from the "all" target, containing the original sources and with the same properties as the actually built targets - that are compiled with unified sources - and use these targets only to add entries in compile_commands.json file. I would be something like `add_library(JavaScriptCoreOriginalSources ... EXCLUDE_FROM_ALL)`. Hopefully a macro in the right place would do it well. This way it would get changes introduced during development without manual interventation. Of course enabling this only under an option. Haven't tested this approach yet. Personally I don't like either option too much. Any thoughts on the ideas or another idea? Comment on attachment 339382 [details]
Patch
This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.
|