New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 743313 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

inspector_protocol/templates/TypeBuilder_h.template breaks with Jinja 2.9

Reported by w...@extrahop.com, Jul 15 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

Steps to reproduce the problem:
1. Attempt to build V8 on a system with Jinja 2.9 (it appears Jinja is not bundled with V8 even if it is bundled with Chromium?)

What is the expected behavior?
Successful build

What went wrong?
Build failure. See https://bugs.gentoo.org/show_bug.cgi?id=605140 for example.

Did this work before? N/A 

Chrome version:   Channel: n/a
OS Version: 
Flash Version: 

inspector_protocol/templates/TypeBuilder_h.template is using Jinja assignments incorrectly. I believe the fix looks like this (line numbers are from the version of inspector_protocol in V8 6.0.186):

@@ -110,12 +110,8 @@ public:
     public:
         enum {
             NoFieldsSet = 0,
-    {% set count = 0 %}
-    {% for property in type.properties %}
-      {% if not(property.optional) %}
-        {% set count = count + 1 %}
-          {{property.name | to_title_case}}Set = 1 << {{count}},
-      {% endif %}
+    {% for property in type.properties|rejectattr("optional") %}
+      {{property.name | to_title_case}}Set = 1 << {{loop.index}},
     {% endfor %}
             AllFieldsSet = (
     {%- for property in type.properties %}

The Jinja documentation (at least for 2.9) suggests that this code was always buggy:

Scoping Behavior

Please keep in mind that it is not possible to set variables inside a block and have them show up outside of it. This also applies to loops. The only exception to that rule are if statements which do not introduce a scope. As a result the following template is not going to do what you might expect:

{% set iterated = false %}
{% for item in seq %}
    {{ item }}
    {% set iterated = true %}
{% endfor %}
{% if not iterated %} did not iterate {% endif %}

It is not possible with Jinja syntax to do this. Instead use alternative constructs like the loop else block or the special loop variable:

{% for item in seq %}
    {{ item }}
{% else %}
    did not iterate
{% endfor %}

Sorry for the cursory bug report. It's late on a Friday...
 
Labels: TE-NeedsTriageHelp

Comment 2 by caseq@chromium.org, Jul 17 2017

Cc: caseq@chromium.org pfeldman@chromium.org
Owner: dgozman@chromium.org
Status: Assigned (was: Unconfirmed)
Owner: kozyatinskiy@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/deps/inspector_protocol/+/8cb7a4f50ff7d5b1b7f2e5df0542dc577c88bdc3

commit 8cb7a4f50ff7d5b1b7f2e5df0542dc577c88bdc3
Author: Alexey Kozyatinskiy <kozyatinskiy@chromium.org>
Date: Wed Oct 11 21:32:17 2017

[inspector_protocol] fixed compatibility with latest jinja 2.9.6

Bug: chromium:743313 
Change-Id: I70e06837aca6acef5c0d658f46f66df97223097a
R=dgozman@chromium.org

[modify] https://crrev.com/8cb7a4f50ff7d5b1b7f2e5df0542dc577c88bdc3/templates/TypeBuilder_h.template

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10

commit 744b49ef0d1b72de2f55994c38ff0d4dea6a2d10
Author: Alexey Kozyatinskiy <kozyatinskiy@chromium.org>
Date: Wed Oct 11 22:52:17 2017

Roll third_party/inspector_protocol to 8cb7a4f50ff7d5b1b7f2e5df0542dc577c88bdc3

This roll includes:
- [inspector_protocol] fixed compatibility with latest jinja 2.9.6
- [inspector_protocol] removed unused variable
- Follow up on alph's review comments.
- Provide default escape implementation for latin and wide strings.
- Allow escaping utf8 strings in embedders that operate std::string.
- Upload inspector_protocol changes to Gerrit by default
- [inspector_protocol] Fix building with non-ASCII paths
- [inspector_protocol] added StringUtil::toDouble method as requirement
- Add const char* overloads to ErrorSupport

BUG= chromium:743313 
R=dgozman@chromium.org

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ic81a62c638bf592ae65c84055d53d926e50715ac
Reviewed-on: https://chromium-review.googlesource.com/713538
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48477}
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/src/inspector/string-util.cc
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/src/inspector/string-util.h
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/CodeGenerator.py
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/README.v8
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/DispatcherBase_cpp.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/DispatcherBase_h.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/ErrorSupport_cpp.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/ErrorSupport_h.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/Values_cpp.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/lib/Values_h.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/templates/TypeBuilder_cpp.template
[modify] https://crrev.com/744b49ef0d1b72de2f55994c38ff0d4dea6a2d10/third_party/inspector_protocol/templates/TypeBuilder_h.template

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 12 2017

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

commit 79699738fbe646f3a68c3f33b97544ed83b37a72
Author: Alexey Kozyatinskiy <kozyatinskiy@chromium.org>
Date: Thu Oct 12 14:43:21 2017

Roll third_party/inspector_protocol to 8cb7a4f50ff7d5b1b7f2e5df0542dc577c88bdc3

This roll includes:
- [inspector_protocol] fixed compatibility with latest jinja 2.9.6
- [inspector_protocol] removed unused variable
- [inspector_protocol] Fix building with non-ASCII paths

R=dgozman@chromium.org

Bug:  chromium:743313 
Change-Id: I998a02f6c97ebc82ff5de5d202b5532d3aea5066
Reviewed-on: https://chromium-review.googlesource.com/714236
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508347}
[modify] https://crrev.com/79699738fbe646f3a68c3f33b97544ed83b37a72/headless/lib/browser/devtools_api/domain_types_h.template
[modify] https://crrev.com/79699738fbe646f3a68c3f33b97544ed83b37a72/third_party/inspector_protocol/CodeGenerator.py
[modify] https://crrev.com/79699738fbe646f3a68c3f33b97544ed83b37a72/third_party/inspector_protocol/README.chromium
[modify] https://crrev.com/79699738fbe646f3a68c3f33b97544ed83b37a72/third_party/inspector_protocol/templates/TypeBuilder_h.template

Status: Fixed (was: Assigned)

Sign in to add a comment