New issue
Advanced search Search tips

Issue 652018 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 563793



Sign in to add a comment

build/android/gyp/java_cpp_enum.py gets confused by wrapped lines

Project Member Reported by dcheng@chromium.org, Oct 1 2016

Issue description

clang-format wraps long lines like this:

KeyModifiers = SymbolKey | FnKey | AltGrKey | MetaKey | AltKey | ControlKey | ShiftKey

into

KeyModifiers =
    SymbolKey | FnKey | AltGrKey | MetaKey | AltKey | ControlKey | ShiftKey

But that confuses the generation of java enums from C++ source files.

Someone from android should probably look at this, for now, we're going to disable clang formatting for that one line
 
Cc: agrieve@chromium.org
Fails like so before the workaround (https://codereview.chromium.org/2378283006/):

FAILED: gen/third_party/WebKit/public/blink_headers_java_enums_srcjar.srcjar 
python ../../build/android/gyp/java_cpp_enum.py --depfile gen/third_party/WebKit/public/blink_headers_java_enums_srcjar.d --srcjar=gen/third_party/WebKit/public/blink_headers_java_enums_srcjar.srcjar ../../third_party/WebKit/public/platform/WebDisplayMode.h ../../third_party/WebKit/public/platform/WebInputEvent.h ../../third_party/WebKit/public/web/WebTextInputType.h
Traceback (most recent call last):
  File "../../build/android/gyp/java_cpp_enum.py", line 367, in <module>
    DoMain(sys.argv[1:])
  File "../../build/android/gyp/java_cpp_enum.py", line 359, in DoMain
    for output_path, data in DoGenerate(input_paths):
  File "../../build/android/gyp/java_cpp_enum.py", line 250, in DoGenerate
    enum_definitions = DoParseHeaderFile(source_path)
  File "../../build/android/gyp/java_cpp_enum.py", line 266, in DoParseHeaderFile
    return HeaderParser(f.readlines(), path).ParseDefinitions()
  File "../../build/android/gyp/java_cpp_enum.py", line 169, in ParseDefinitions
    self._ParseLine(line)
  File "../../build/android/gyp/java_cpp_enum.py", line 178, in _ParseLine
    self._ParseEnumLine(line)
  File "../../build/android/gyp/java_cpp_enum.py", line 198, in _ParseEnumLine
    self._current_definition.AppendEntry(enum_key, enum_value)
  File "../../build/android/gyp/java_cpp_enum.py", line 41, in AppendEntry
    raise Exception('Multiple definitions of key %s found.' % key)
Exception: Multiple definitions of key SymbolKey found.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b6ae86a409be490daa69803c62dcd0d6bee7b303

commit b6ae86a409be490daa69803c62dcd0d6bee7b303
Author: Daniel Cheng <dcheng@chromium.org>
Date: Sat Oct 01 00:56:58 2016

Add a clang-format exception to WebInputEvent::Modifiers.

build/android/gyp/java_cpp_enum.py gets confused when clang-format wraps
the excessively long line.

BUG= 652018 
R=thakis@chromium.org

Review URL: https://codereview.chromium.org/2378283006 .

Cr-Commit-Position: refs/heads/master@{#422258}

[modify] https://crrev.com/b6ae86a409be490daa69803c62dcd0d6bee7b303/third_party/WebKit/public/platform/WebInputEvent.h

Cc: estevenson@chromium.org
Cc: -estevenson@chromium.org
Owner: estevenson@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9066edf6a2199aa3390cc55e89a93f858b50761b

commit 9066edf6a2199aa3390cc55e89a93f858b50761b
Author: estevenson <estevenson@chromium.org>
Date: Tue Oct 04 18:27:32 2016

Allow multi-line enum entries in java_cpp_enum.py.

clang-format wraps long lines, so java_cpp_enum.py should also
handle multi-line enum entries.

Also changed the HeaderParser to ignore empty comments.

BUG= 652018 

Review-Url: https://codereview.chromium.org/2396533002
Cr-Commit-Position: refs/heads/master@{#422863}

[modify] https://crrev.com/9066edf6a2199aa3390cc55e89a93f858b50761b/build/android/gyp/java_cpp_enum.py
[modify] https://crrev.com/9066edf6a2199aa3390cc55e89a93f858b50761b/build/android/gyp/java_cpp_enum_tests.py

Status: Fixed (was: Assigned)
Status: Started (was: Fixed)
Thanks! Can you revert https://codereview.chromium.org/2378283006 too now that this works?
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc0da208fe58bbb9a4fa3290cdae739c2511ad1c

commit cc0da208fe58bbb9a4fa3290cdae739c2511ad1c
Author: estevenson <estevenson@chromium.org>
Date: Wed Oct 05 22:56:42 2016

Revert of Add a clang-format exception to WebInputEvent::Modifiers. (patchset #1 id:1 of https://codereview.chromium.org/2378283006/ )

Reason for revert:
crbug/652018 is fixed; build/android/gyp/java_cpp_enum.py handles multi-line enum entries so the clang-format exception is no longer necessary.

Original issue's description:
> Add a clang-format exception to WebInputEvent::Modifiers.
>
> build/android/gyp/java_cpp_enum.py gets confused when clang-format wraps
> the excessively long line.
>
> BUG= 652018 
> R=thakis@chromium.org
>
> Committed: https://chromium.googlesource.com/chromium/src/+/b6ae86a409be490daa69803c62dcd0d6bee7b303

TBR=thakis@chromium.org,dcheng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 652018 

Review-Url: https://codereview.chromium.org/2393183002
Cr-Commit-Position: refs/heads/master@{#423326}

[modify] https://crrev.com/cc0da208fe58bbb9a4fa3290cdae739c2511ad1c/third_party/WebKit/public/platform/WebInputEvent.h

Status: Fixed (was: Started)
Blocking: 563793
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc0da208fe58bbb9a4fa3290cdae739c2511ad1c

commit cc0da208fe58bbb9a4fa3290cdae739c2511ad1c
Author: estevenson <estevenson@chromium.org>
Date: Wed Oct 05 22:56:42 2016

Revert of Add a clang-format exception to WebInputEvent::Modifiers. (patchset #1 id:1 of https://codereview.chromium.org/2378283006/ )

Reason for revert:
crbug/652018 is fixed; build/android/gyp/java_cpp_enum.py handles multi-line enum entries so the clang-format exception is no longer necessary.

Original issue's description:
> Add a clang-format exception to WebInputEvent::Modifiers.
>
> build/android/gyp/java_cpp_enum.py gets confused when clang-format wraps
> the excessively long line.
>
> BUG= 652018 
> R=thakis@chromium.org
>
> Committed: https://chromium.googlesource.com/chromium/src/+/b6ae86a409be490daa69803c62dcd0d6bee7b303

TBR=thakis@chromium.org,dcheng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 652018 

Review-Url: https://codereview.chromium.org/2393183002
Cr-Commit-Position: refs/heads/master@{#423326}

[modify] https://crrev.com/cc0da208fe58bbb9a4fa3290cdae739c2511ad1c/third_party/WebKit/public/platform/WebInputEvent.h

Comment 13 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
I faced another conflict between git cl format and generated Java enums today when the package name was longer than 80 chars:

// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.blink_public.platform.modules.remoteplayback

git cl format wrapped the line and broke java_cpp_enum.

By staring at java_cpp_enum.py I found the continuation regexp (hope) and then its test revealed the continuation trick is by using () brackets like so:

// GENERATED_JAVA_ENUM_PACKAGE: (
//     org.chromium.blink_public.platform.modules.remoteplayback)

This is not documented anywhere though (the only documentation for this feature I found was the announcement thread 2 years ago: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/3xr44Rk_cHQ).

Can "git cl format" use this workaround for GENERATED_JAVA_ prefixes in comments?

Perhaps changing the error message in java_cpp_enum.py like below:

'Did you forget prefixing enums with "// GENERATED_JAVA_ENUM_PACKAGE: foo"?' ->
'Did you forget prefixing enums with "// GENERATED_JAVA_ENUM_PACKAGE: foo" or use () if "foo" is on the other line?'

is the easiest and good enough fix?

sgtm to just update the error message.
Status: Assigned (was: Fixed)
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9c44c94152587aea8b0d4a83f0df00398c2a5bee

commit 9c44c94152587aea8b0d4a83f0df00398c2a5bee
Author: estevenson <estevenson@chromium.org>
Date: Wed Nov 16 18:24:51 2016

Update error message in java_cpp_enum.py.

The new error message explains how to use brackets with multi-line
directive declarations, which is useful since this feature isn't
documented elsewhere.

Also changed the default value of |_path| in |HeaderParser| since it was
causing tests to raise exceptions because of a NoneType error.

BUG= 652018 

Review-Url: https://codereview.chromium.org/2505703002
Cr-Commit-Position: refs/heads/master@{#432562}

[modify] https://crrev.com/9c44c94152587aea8b0d4a83f0df00398c2a5bee/build/android/gyp/java_cpp_enum.py
[modify] https://crrev.com/9c44c94152587aea8b0d4a83f0df00398c2a5bee/build/android/gyp/java_cpp_enum_tests.py
[modify] https://crrev.com/9c44c94152587aea8b0d4a83f0df00398c2a5bee/net/base/request_priority.h

Status: Fixed (was: Assigned)

Sign in to add a comment