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

Issue 866232 link

Starred by 1 user
Status: Fixed
Owner:
alemate@chromium.org
Closed: Jul 25
Cc:
flackr@chromium.org
agrieve@chromium.org
thestig@chromium.org
█ rsgingerrs@chromium.org
thakis@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

grit incorrectly generates depfile.

Project Member Reported by alemate@chromium.org, Jul 21 
Grit ignores conditions when generating dep file:                                
                                                                                 
https://chromium-review.googlesource.com/c/chromium/src/+/1144272/1/chrome/browser/resources/chromeos/login/app_downloading.html
                                                                                 
This CL fails on chromium trybot, because "../../../internal/resources/arc_support/videos/app_downloading.mp4" will not be included in result (as conditions do not satisfy), but will be included in dep file:
                                                                                 
python ../../../../src/chromium/src/tools/grit/grit.py -i ../../../../src/chromium/src/chrome/browser/browser_resources.grd build -o gen/chrome --depdir . --depfile gen/chrome/resources_stamp.d --write-only-new=1 --depend-on-stamp -D _chromium -E CHROMIUM_BUILD=chromium -D chromeos -D scale_factors=2x -D toolkit_views -D use_aura -D use_nss_certs -D use_ozone -D enable_app_list=true -D enable_arcore=false -D enable_background_mode=false -D enable_background_contents=true -D enable_extensions=true -D enable_google_now=false -D enable_hangout_services_extension=false -D enable_plugins=true -D enable_print_preview=true -D enable_printing=true -D enable_service_discovery=true -D enable_vr=false -D mac_views_browser=false -D safe_browsing_mode=1 -D optimize_webui=true -E additional_modules_list_file=gen/chrome/browser/internal/additional_modules_list.txt -E root_gen_dir=gen -f ../../../../src/chromium/src/tools/gritsettings/resource_ids --assert-file-list=obj/chrome/browser/resources_expected_outputs.txt
                                                                                 
                                                                                 
This command will include "../../../internal/resources/arc_support/videos/app_downloading.mp4"  to gen/chrome/resources_stamp.d file.
                                                                                 
The problem is here:                                                        
                                                                                 
  Traceback (most recent call last):                                             
    File "../../../../src/chromium/src/tools/grit/grit.py", line 15, in <module> 
      grit.grit_runner.Main(sys.argv[1:])                                        
    File "tools/grit/grit/grit_runner.py", line 249, in Main                     
      toolobject.Run(options, args[1:])                                            
    File "tools/grit/grit/tool/build.py", line 264, in Run                        
      self.GenerateDepfile(depfile, depdir, first_ids_file, depend_on_stamp)       
    File "tools/grit/grit/tool/build.py", line 504, in GenerateDepfile            
      infiles = self.res.GetInputFiles()                                           
    File "tools/grit/grit/node/misc.py", line 470, in GetInputFiles              
      input_files.update(node.GetHtmlResourceFilenames())                          
    File "tools/grit/grit/node/structure.py", line 205, in GetHtmlResourceFilenames
      return self.gatherer.GetHtmlResourceFilenames()                              
    File "tools/grit/grit/gather/chrome_html.py", line 327, in GetHtmlResourceFilenames
      filename_expansion_function=self.filename_expansion_function)                
    File "tools/grit/grit/format/html_inline.py", line 584, in GetResourceFilenames
      filename_expansion_function=filename_expansion_function).inlined_files       
    File "tools/grit/grit/format/html_inline.py", line 487, in DoInline          
      flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text)                   
    File "tools/grit/grit/format/html_inline.py", line 403, in InlineIncludeFiles
      return InlineFileContents(src_match, '%s')                                   
    File "tools/grit/grit/format/html_inline.py", line 385, in InlineFileContents
      filename_expansion_function=filename_expansion_function))                    
    File "tools/grit/grit/format/html_inline.py", line 584, in GetResourceFilenames
      filename_expansion_function=filename_expansion_function).inlined_files       
    File "tools/grit/grit/format/html_inline.py", line 485, in DoInline          
      flat_text = CheckConditionalElements(flat_text)


this will evaluate "chromeos and _google_chrome" as TRUE. In the same grit run the same condition is FALSE here:

Traceback (most recent call last):                                               
  File "../../../../src/chromium/src/tools/grit/grit.py", line 15, in <module>   
    grit.grit_runner.Main(sys.argv[1:])                                          
  File "tools/grit/grit/grit_runner.py", line 249, in Main                       
    toolobject.Run(options, args[1:])                                            
  File "tools/grit/grit/tool/build.py", line 249, in Run                         
    self.res.RunGatherers()                                                      
  File "tools/grit/grit/node/misc.py", line 651, in RunGatherers                 
    node.RunPreSubstitutionGatherer(debug=debug)                                 
  File "tools/grit/grit/node/structure.py", line 224, in RunPreSubstitutionGatherer 
    self.gatherer.Parse()                                                        
  File "tools/grit/grit/gather/chrome_html.py", line 357, in Parse               
    filename_expansion_function=self.filename_expansion_function)                
  File "tools/grit/grit/format/html_inline.py", line 545, in InlineToString      
    filename_expansion_function=filename_expansion_function).inlined_data        
  File "tools/grit/grit/format/html_inline.py", line 487, in DoInline            
    flat_text = _INCLUDE_RE.sub(InlineIncludeFiles, flat_text)                   
  File "tools/grit/grit/format/html_inline.py", line 403, in InlineIncludeFiles  
    return InlineFileContents(src_match, '%s')                                   
  File "tools/grit/grit/format/html_inline.py", line 392, in InlineFileContents  
    filename_expansion_function=filename_expansion_function)                     
  File "tools/grit/grit/format/html_inline.py", line 485, in DoInline            
    flat_text = CheckConditionalElements(flat_text)

Comment 1 by alemate@chromium.org, Jul 21

Labels: ReleaseBlock-Stable M-69
I don't know grit architecture, could somebody fro OWNERS take a look?

This is actually blocking a few UI tasks for M69.

Comment 2 by thakis@chromium.org, Jul 21 

If you need this, you'll have to try and fix it yourself.

Comment 3 by thakis@chromium.org, Jul 21 

(To provide some context, none of the current grit owners have written grit. We've all written some patches to it and know it well enough to review changes to it, but that's about it.)
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 24 

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by gov...@chromium.org, Jul 24 

Pls apply appropriate OSs label. Thank you.

Comment 6 by alemate@chromium.org, Jul 24

Labels: OS-Chrome

Comment 7 by alemate@chromium.org, Jul 24

Owner: alemate@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 25 

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

commit afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5
Author: Alexander Alekseev <alemate@chromium.org>
Date: Wed Jul 25 21:58:36 2018

Grit: Fix bug with generating dependency on hidden files.

Grit previously ignored <if> conditions when generating dependency file.
This CL makes Grit to include into dependency only files that were actually
included in the output.

Bug:  866232 , 852038 
Change-Id: I8d974219d00a76568263d7ecf8eab811807565b4
Reviewed-on: https://chromium-review.googlesource.com/1149145
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578086}
[modify] https://crrev.com/afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5/tools/grit/grit/format/html_inline.py
[modify] https://crrev.com/afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5/tools/grit/grit/format/html_inline_unittest.py
[modify] https://crrev.com/afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5/tools/grit/grit/gather/chrome_html.py
[modify] https://crrev.com/afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5/tools/grit/grit/node/include.py

Comment 9 by alemate@chromium.org, Jul 25

Labels: Merge-Request-69
Status: Fixed (was: Started)
Project Member

Comment 10 by sheriffbot@chromium.org, Jul 26

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 27

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/88695c64b41b51f02e484b0b7bee3db356eab9fb

commit 88695c64b41b51f02e484b0b7bee3db356eab9fb
Author: Alexander Alekseev <alemate@chromium.org>
Date: Fri Jul 27 00:45:57 2018

Grit: Fix bug with generating dependency on hidden files.

Grit previously ignored <if> conditions when generating dependency file.
This CL makes Grit to include into dependency only files that were actually
included in the output.

TBR=alemate@chromium.org

(cherry picked from commit afa32f3e6933cfa8bb93af265d77f5d4e0cda9e5)

Bug:  866232 , 852038 
Change-Id: I8d974219d00a76568263d7ecf8eab811807565b4
Reviewed-on: https://chromium-review.googlesource.com/1149145
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Alexander Alekseev <alemate@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#578086}
Reviewed-on: https://chromium-review.googlesource.com/1152523
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#136}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/88695c64b41b51f02e484b0b7bee3db356eab9fb/tools/grit/grit/format/html_inline.py
[modify] https://crrev.com/88695c64b41b51f02e484b0b7bee3db356eab9fb/tools/grit/grit/format/html_inline_unittest.py
[modify] https://crrev.com/88695c64b41b51f02e484b0b7bee3db356eab9fb/tools/grit/grit/gather/chrome_html.py
[modify] https://crrev.com/88695c64b41b51f02e484b0b7bee3db356eab9fb/tools/grit/grit/node/include.py
►

Sign in to add a comment