SublimeClang indexing broken with "tu is None..." by SK_FREETYPE_MINIMUM_RUNTIME_VERSION define |
|||
Issue descriptionChrome ToT r478712, on Linux with target_os=chromeos * Set up SublimeClang per https://chromium.googlesource.com/chromium/src/+/master/docs/linux_sublime_dev.md * Open base/base_switches.cc * Good: Everything is indexed, autocomplete is smart, syntax errors are highlighted * Open ash/ash_switches.cc * Bad: Nothing is indexed, syntax errors are not highlighted Will compile file /w/chrome/src/ash/ash_switches.cc with the following options: ['-I/usr/local/google/home/jamescook/.config/sublime-text-3/Packages/SublimeClang/internals/clang/include', '-Wno-attributes', '-x', 'c++', '-std=c++11', '-DUSE_CLANG_COMPLETER', '-std=c++11', '-x', 'c++', '-I/w/chrome/src', '-Wno-unknown-warning-option', '-DASH_IMPLEMENTATION', '-DV8_DEPRECATION_WARNINGS', '-DDCHECK_ALWAYS_ON=1', '-DUSE_UDEV', '-DUSE_ASH=1', '-DUSE_AURA=1', '-DUSE_NSS_CERTS=1', '-DUSE_OZONE=1', '-DDISABLE_NACL', '-DFULL_SAFE_BROWSING', '-DSAFE_BROWSING_CSD', '-DSAFE_BROWSING_DB_LOCAL', '-DCHROMIUM_BUILD', '-DFIELDTRIAL_TESTING_ENABLED', '-DCR_CLANG_REVISION=303910-1', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE_SOURCE', '-D_LARGEFILE64_SOURCE', '-D__STDC_CONSTANT_MACROS', '-D__STDC_FORMAT_MACROS', '-DCOMPONENT_BUILD', '-DOS_CHROMEOS', '-DNDEBUG', '-DNVALGRIND', '-DDYNAMIC_ANNOTATIONS_ENABLED=0', '-DGL_GLEXT_PROTOTYPES', '-DUSE_GLX', '-DUSE_EGL', '-DANGLE_ENABLE_RELEASE_ASSERTS', '-DTOOLKIT_VIEWS=1', '-DU_USING_ICU_NAMESPACE=0', '-DU_ENABLE_DYLOAD=0', '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE', '-DUCHAR_TYPE=uint16_t', '-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS', '-DSK_HAS_PNG_LIBRARY', '-DSK_HAS_WEBP_LIBRARY', '-DSK_HAS_JPEG_LIBRARY', '-DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR)', '*', '0x01000000)', '|', '((FREETYPE_MINOR)', '*', '0x00010000)', '|', '((FREETYPE_PATCH)', '*', '0x00000100))', '-DSKIA_DLL', '-DGR_GL_IGNORE_ES3_MSAA=0', '-DSK_SUPPORT_GPU=1', '-DGOOGLE_PROTOBUF_NO_RTTI', '-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER', '-DHAVE_PTHREAD', '-DPROTOBUF_USE_DLLS', '-DMESA_EGL_NO_X11_HEADERS', '-DBORINGSSL_SHARED_LIBRARY', '-DUSING_V8_SHARED', '-I/w/chrome/src', '-I/w/chrome/src/out/Default/gen', '-I/w/chrome/src/third_party/libwebp', '-I/w/chrome/src/third_party/khronos', '-I/w/chrome/src/gpu', '-I/w/chrome/src/third_party/ced/src', '-I/w/chrome/src/third_party/icu/source/common', '-I/w/chrome/src/third_party/icu/source/i18n', '-I/w/chrome/src/skia/config', '-I/w/chrome/src/skia/ext', '-I/w/chrome/src/third_party/skia/include/c', '-I/w/chrome/src/third_party/skia/include/config', '-I/w/chrome/src/third_party/skia/include/core', '-I/w/chrome/src/third_party/skia/include/effects', '-I/w/chrome/src/third_party/skia/include/encode', '-I/w/chrome/src/third_party/skia/include/images', '-I/w/chrome/src/third_party/skia/include/lazy', '-I/w/chrome/src/third_party/skia/include/pathops', '-I/w/chrome/src/third_party/skia/include/pdf', '-I/w/chrome/src/third_party/skia/include/pipe', '-I/w/chrome/src/third_party/skia/include/ports', '-I/w/chrome/src/third_party/skia/include/utils', '-I/w/chrome/src/third_party/skia/third_party/vulkan', '-I/w/chrome/src/third_party/skia/include/gpu', '-I/w/chrome/src/third_party/skia/src/gpu', '-I/w/chrome/src/third_party/skia/src/sksl', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/dbus-1.0', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include', '-I/w/chrome/src/third_party/protobuf/src', '-I/w/chrome/src/out/Default/gen/protoc_out', '-I/w/chrome/src/third_party/protobuf/src', '-I/w/chrome/src/third_party/mesa/src/include', '-I/w/chrome/src/third_party/libwebm/source', '-I/w/chrome/src/third_party/boringssl/src/include', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/nss', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/nspr', '-I/w/chrome/src/third_party/WebKit', '-I/w/chrome/src/out/Default/gen/third_party/WebKit', '-I/w/chrome/src/v8/include', '-I/w/chrome/src/out/Default/gen/v8/include', '-I/w/chrome/src/third_party/qcms/src', '-I/w/chrome/src/out/Default/gen', '-fno-strict-aliasing', '-Wno-builtin-macro-redefined', '-D__DATE__=', '-D__TIME__=', '-D__TIMESTAMP__=', '-funwind-tables', '-fPIC', '-fcolor-diagnostics', '-m64', '-march=x86-64', '-Wall', '-Werror', '-Wextra', '-Wno-missing-field-initializers', '-Wno-unused-parameter', '-Wno-shift-negative-value', '-Wno-c++11-narrowing', '-Wno-covered-switch-default', '-Wno-unneeded-internal-declaration', '-Wno-inconsistent-missing-override', '-Wno-undefined-var-template', '-Wno-nonportable-include-path', '-Wno-address-of-packed-member', '-Wno-unused-lambda-capture', '-Wno-user-defined-warnings', '-O2', '-fno-ident', '-fdata-sections', '-ffunction-sections', '-fno-omit-frame-pointer', '--sysroot=/w/chrome/src/build/linux/debian_jessie_amd64-sysroot', '-fvisibility=hidden', '-Wheader-hygiene', '-Wstring-conversion', '-Wtautological-overlap-compare', '-std=gnu++11', '-fno-rtti', '-fno-exceptions', '-fvisibility-inlines-hidden'] tu is None... Git bisect shows it started with this CL: Author: Ben Wagner <bungeman@chromium.org> Date: Mon Jun 5 14:45:34 2017 -0400 Set build flag when using own FreeType. Set the SK_FREETYPE_MINIMUM_RUNTIME_VERSION build flag when the use_system_freetype gn flag is false. Currently the build flag is only set when is_win or is_mac, but this needs to be set any time it is known that FreeType will be built into the resulting binary. In particular this is desired for Android and WebView, but also for desktop Linux. BUG= chromium:715429 Change-Id: Ie5e425b0b347caf51400c5522a42e8a88605c147 Reviewed-on: https://chromium-review.googlesource.com/524185 It causes the list of defines to have an item that has spaces in it. defines += [ "SK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR) * 0x01000000) | ((FREETYPE_MINOR) * 0x00010000) | ((FREETYPE_PATCH) * 0x00000100))" ] which some part of the SublimeClang tooling turns into a python list, chunked by spaces: '-DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR)', '*', '0x01000000)', '|', '((FREETYPE_MINOR)', '*', '0x00010000)', '|', '((FREETYPE_PATCH)', '*', '0x00000100))' and '*' isn't a valid clang flag, so indexing dies. You'll only see the indexing failure on files that have skia as a dependency (so it'll show up for files in //ash and //chrome but not //base). This define seems really weird -- not only does it have spaces, but it's doing math. bungemen, is there some other way we can compute this define value? scottmg suggested generating a header.
,
Jun 13 2017
Taking a quick look at https://github.com/quarnster/SublimeClang/blob/6823e7f0904e60680ac9f898108e301582ec5505/internals/translationunitcache.py print("Will compile file %s with the following options:\n%s" % (filename, opts)) it appears that the output of the sublimeclang_options_script is read in as opts += shlex.split(bdecode(output[0])) where bdecode / sencode are just internal utf-8 handlers. However, just running python tools/sublime/ninja_options_script.py -d '~/src/depot_tools' third_party/skia/src/ports/SkFontHost_FreeType.cpp shows that while the chromium.ycm_extra_conf.py parsed everything fine, ninja_options_script.py doesn't write out this information in the format expected by the plugin (it doesn't shell escape it). I think this can be fixed by use of 'print pipes.quote(flag)' (or shlex.quote if only python 3). Note that '-DCR_CLANG_REVISION=303910-1' also should be escaped, it should actually be '-DCR_CLANG_REVISION="303910-1"' (it has quotes).
,
Jun 13 2017
I've created https://chromium-review.googlesource.com/c/533453/ which should fix this, but I don't use sublime, so someone who does should review.
,
Jun 13 2017
Thanks for the analysis and writing the patch. Your CL works for me in Sublime and fixes my problem.
,
Jun 13 2017
Note that there is also no reason for this define to show up when compiling ash/ash_switches.cc, it should only be set internally when building Skia. I've created https://chromium-review.googlesource.com/c/533573/ for that.
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5d08c248cd71d45eebb97859ba172a5c882f1a0c commit 5d08c248cd71d45eebb97859ba172a5c882f1a0c Author: Ben Wagner <bungeman@chromium.org> Date: Tue Jun 13 16:49:31 2017 sublime/ninja_options_script.py shell quote output. The sublimeclang plugin expects its input to be parseable by shlex. The current output of ninja_options_script.py it not escaped, leading to incorrect parsing. This changes ninja_options_script.py to escape its output for parsing by shlex. BUG= chromium:732670 Change-Id: Ib57a8c15a3cc78df83ee79fa8d92a04b57531572 Reviewed-on: https://chromium-review.googlesource.com/533453 Commit-Queue: Ben Wagner <bungeman@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#479051} [modify] https://crrev.com/5d08c248cd71d45eebb97859ba172a5c882f1a0c/tools/sublime/ninja_options_script.py
,
Jun 13 2017
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d96b8285c1d57854c6679c6d3e9dbef1e683d1b commit 6d96b8285c1d57854c6679c6d3e9dbef1e683d1b Author: Ben Wagner <bungeman@chromium.org> Date: Tue Jun 13 22:29:25 2017 Make SK_FREETYPE_MINIMUM_RUNTIME_VERSION private. The define SK_FREETYPE_MINIMUM_RUNTIME_VERSION is only used when building Skia the library -- it does not affect any headers. As a result, it should only be set when building Skia, it does not need to be defined when building dependencies. BUG= chromium:732670 Change-Id: I35eb988fb9e8af395e9f5d8668d7c0b3a5a02db8 Reviewed-on: https://chromium-review.googlesource.com/533573 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Ben Wagner <bungeman@google.com> Cr-Commit-Position: refs/heads/master@{#479178} [modify] https://crrev.com/6d96b8285c1d57854c6679c6d3e9dbef1e683d1b/skia/BUILD.gn
,
Jul 14 2017
Could you please merge this in M60. We are seeing this issue in while building 60.0.3112.66 but not on Master.
,
Jul 14 2017
I'm assuming the change you're looking to merge to M60 is the one from Comment 6, which is just the change to the sublimeclang project generator? (There is also the change in Comment 8, which is just a build clean up.)
,
Jul 14 2017
bungeman, can you sort out the merge issue here? I didn't actually do anything with this bug other than file it. :-) We're pretty close to the stable cut. OTOH, if all we want is the sublime script, that seems safe to merge.
,
Jul 14 2017
We merge stuff to stable to ship a product. Why would we merge a change to an editor integration?
,
Jul 18 2017
Apologies for not following up promptly. I was referring to comment#8 commit. While building M60 branch, I was facing build issues and cherry-picking commit#8 change seemed to fix the problem.
,
Jul 19 2017
The change in comment #8 should have been purely cosmetic in nature, it simply causes a particular define be passed to the compiler for fewer translation units. For most translation units having this define set was just a no-op; outside of Skia the text SK_FREETYPE_MINIMUM_RUNTIME_VERSION doesn't appear, so the define is never actually used outside Skia code. I'm unfamiliar with what build may have had errors and don't know what the build errors actually were. However, the Chromium / Chrome build system seems to be fine with how things were in M60. As a result I can only assume that the failing build contains some kind of transformation which doesn't properly escape defines (as tools/sublime/ninja_options_script.py did before the change in comment #6). At this point I think it would be pretty difficult to convince the merge team to approve merging a build change like comment #8. It doesn't fix an actual bug in Chromium itself (it doesn't cause changes to any code), you have a work-around, and there aren't enough details here to rule out that the issue isn't an external build process improperly escaping defines. |
|||
►
Sign in to add a comment |
|||
Comment 1 by jamescook@chromium.org
, Jun 13 2017