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

Issue 750406 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
On parental leave until 3/15/19
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

Some PRESUBMIT.py checks are not run on windows due to file separator differences

Project Member Reported by dougt@chromium.org, Jul 29 2017

Issue description

In our PRESUBMIT.py files, it is common to have a pattern like:

THE_FILE_I_CARE_ABOUT = 'content/whatever/something.idl'
...
if THE_FILE_I_CARE_ABOUT in input_api.LocalPaths():
  DoSomeCheck(input_api, output_api)


Notice, that we're defining the file path we care about with UNIX style separators and we're comparing that string to strings returned by LocalPaths().  Strings returned by LocalPaths() will always have platform style paths.  This means that on the mac and on linux, all is fine.  But on Window, these sorts of PRESUMIT.py checks will not run because windows uses '\' as a file path.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 7 2017

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

commit 4430d79c1ba85e58ef9fc12a95d3f729b7c0e24b
Author: Doug Turner <dougt@chromium.org>
Date: Mon Aug 07 21:00:40 2017

Enable ui\accessiblity PRESUBMIT.py check on windows.

Due to file separator differences, the PRESUMIT.py would never run on
windows. This CL simply converts windows path seperators to unix style
ones.

Bug:  750406 
Change-Id: I232dfaa88a04430de935b23c5d115f97b2b5af3e
Reviewed-on: https://chromium-review.googlesource.com/594911
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Doug Turner <dougt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492404}
[modify] https://crrev.com/4430d79c1ba85e58ef9fc12a95d3f729b7c0e24b/ui/accessibility/PRESUBMIT.py

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 7 2017

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

commit 3e82be64bb15f9176e2af945bb995d69f200f606
Author: Doug Turner <dougt@chromium.org>
Date: Mon Aug 07 22:06:57 2017

Enable tools/perf_expectations PRESUBMIT.py check on windows.

Due to file separator differences, the PRESUMIT.py would never run on
windows. This CL simply converts windows path seperators to unix style
ones.

Bug:  750406 
Change-Id: I9523387a350abe70d68918398c65e989055cf27c
Reviewed-on: https://chromium-review.googlesource.com/599121
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Doug Turner <dougt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492428}
[modify] https://crrev.com/3e82be64bb15f9176e2af945bb995d69f200f606/tools/perf_expectations/PRESUBMIT.py

Comment 3 by dougt@chromium.org, Aug 8 2017

Owner: lassey@chromium.org
Quick audit:

PRESUBMITs with this pattern

src
  -- uses basename()

src/ppapi
  -- splitting the file path using os.sep

src/net/tools/dafsa
  -- BUG

src/ui/accessibility
  -- https://chromium-review.googlesource.com/594911c (Fixed)

src/net/tools/ct_log_list
  -- BUG

src/tools/perf_expectations
  -- https://chromium-review.googlesource.com/599121 (Fixed)

src/chrome/common/extensions
  -- Uses os.path.join

src/chrome/browser/resources/chromeos/braille_ime
  -- Only looking at the file extension or name



So, src/net/tools is left and I am not sure if it's that important to run these tools on anything but linux.

Going to punt to lassey for triage.

Comment 4 by lassey@chromium.org, Aug 11 2017

Components: Internals>Network
Owner: sleevi@google.com
Owner: rsleevi@chromium.org
Cc: robpercival@chromium.org rsleevi@chromium.org
Components: -Internals>Network Internals>Network>CertTrans
Owner: ----
These tests should be run on all platforms. I'm not sure when we'd want per-platform PRESUBMITs :)
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 16 2018

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

commit c03c43f3de4ac51a8e04a0582b7122b6868e3e6f
Author: Brad Lassey <lassey@chromium.org>
Date: Tue Jan 16 13:24:38 2018

Presubmit checks don't run on Windows.

    This is because we check local paths against unix style paths.
    This change normalizes the paths of the files we're checking for
    before the comparing to the affected files. This also switches
    to checkingange.AffectedFiles() rather than LocalFiles() because
    one of the files we're looking for isn't in the local directory.

Bug:  750406 
Change-Id: I854f14f6c28b98a13499a9d5b2284e9c769ba5df
Reviewed-on: https://chromium-review.googlesource.com/854412
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Brad Lassey <lassey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529410}
[modify] https://crrev.com/c03c43f3de4ac51a8e04a0582b7122b6868e3e6f/net/tools/ct_log_list/PRESUBMIT.py

Owner: lassey@chromium.org
Status: Verified (was: Available)

Sign in to add a comment