🏗 Add amp check-invalid-whitespaces to AMP's PR check workflow (#33926

)

3p/frame.max.html

@@ -5,7 +5,7 @@
 <script>
 var url = './integration.js';
 try {
-  url = JSON.parse(window.name).bootstrap; 
+  url = JSON.parse(window.name).bootstrap;
 } catch (e) {}
 document.write('<script src="' + url + '"><' + '/script>');
 </script>

amp.js

@@ -43,6 +43,7 @@ createTask('check-analytics-vendors-list', 'checkAnalyticsVendorsList');
 createTask('check-asserts', 'checkAsserts');
 createTask('check-build-system', 'checkBuildSystem');
 createTask('check-exact-versions', 'checkExactVersions');
+createTask('check-invalid-whitespaces', 'checkInvalidWhitespaces');
 createTask('check-links', 'checkLinks');
 createTask('check-owners', 'checkOwners');
 createTask('check-renovate-config', 'checkRenovateConfig');

build-system/pr-check/build-targets.js

@@ -40,6 +40,7 @@ let buildTargets;
 let lintFiles;
 let presubmitFiles;
 let prettifyFiles;
+let invalidWhitespaceFiles;
 
 /***
  * All of AMP's build targets that can be tested during CI.
@@ -54,6 +55,7 @@ const Targets = {
   DOCS: 'DOCS',
   E2E_TEST: 'E2E_TEST',
   INTEGRATION_TEST: 'INTEGRATION_TEST',
+  INVALID_WHITESPACES: 'INVALID_WHITESPACES',
   LINT: 'LINT',
   OWNERS: 'OWNERS',
   PACKAGE_UPGRADE: 'PACKAGE_UPGRADE',
@@ -257,6 +259,13 @@ const targetMatchers = {
       file.startsWith('build-system/server/')
     );
   },
+  [Targets.INVALID_WHITESPACES]: (file) => {
+    return (
+      invalidWhitespaceFiles.includes(file) ||
+      file == 'build-system/tasks/check-invalid-whitespaces.js' ||
+      file.startsWith('build-system/test-configs')
+    );
+  },
   [Targets.UNIT_TEST]: (file) => {
     if (isOwnersFile(file)) {
       return false;
@@ -314,6 +323,7 @@ function determineBuildTargets() {
   lintFiles = globby.sync(config.lintGlobs);
   presubmitFiles = globby.sync(config.presubmitGlobs);
   prettifyFiles = globby.sync(config.prettifyGlobs);
+  invalidWhitespaceFiles = globby.sync(config.invalidWhitespaceGlobs);
   const filesChanged = gitDiffNameOnlyMain();
   for (const file of filesChanged) {
     let isRuntimeFile = true;

build-system/pr-check/checks.js

@@ -28,6 +28,7 @@ const jobName = 'checks.js';
 
 function pushBuildWorkflow() {
   timedExecOrDie('amp presubmit');
+  timedExecOrDie('amp check-invalid-whitespaces');
   timedExecOrDie('amp lint');
   timedExecOrDie('amp prettify');
   timedExecOrDie('amp ava');
@@ -57,6 +58,10 @@ async function prBuildWorkflow() {
     timedExecOrDie('amp presubmit');
   }
 
+  if (buildTargetsInclude(Targets.INVALID_WHITESPACES)) {
+    timedExecOrDie('amp check-invalid-whitespaces');
+  }
+
   if (buildTargetsInclude(Targets.LINT)) {
     timedExecOrDie('amp lint');
   }
@@ -86,9 +91,8 @@ async function prBuildWorkflow() {
     timedExecOrDie('amp dev-dashboard-tests');
   }
 
-  // Validate owners syntax only for PR builds.
   if (buildTargetsInclude(Targets.OWNERS)) {
-    timedExecOrDie('amp check-owners --local_changes');
+    timedExecOrDie('amp check-owners --local_changes'); // only for PR builds
   }
 
   if (buildTargetsInclude(Targets.PACKAGE_UPGRADE)) {

build-system/tasks/check-invalid-whitespaces.js

@@ -0,0 +1,64 @@
+/**
+ * Copyright 2021 The AMP HTML Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+const {getFilesToCheck} = require('../common/utils');
+const {getStdout} = require('../common/process');
+const {green, red} = require('kleur/colors');
+const {invalidWhitespaceGlobs} = require('../test-configs/config');
+const {log, logLocalDev, logWithoutTimestamp} = require('../common/logging');
+
+/**
+ * Runs the check on the given list of files and prints results. Uses the
+ * built-in functionality of `git diff --check` to generate results. Checks
+ * entire files by comparing their contents against an empty commit.
+ *
+ * @param {Array<string>} filesToCheck
+ */
+function runCheck(filesToCheck) {
+  logLocalDev(green('Checking files for invalid whitespaces...'));
+  const fileList = filesToCheck.join(' ');
+  const emptyCommit = getStdout('git hash-object -t tree /dev/null').trim();
+  const checkFileCmd = `git -c color.ui=always diff ${emptyCommit} --check ${fileList}`;
+  const result = getStdout(checkFileCmd).trim();
+  if (result.length) {
+    logWithoutTimestamp(result);
+    log(red('ERROR:'), 'Please fix the files listed above.');
+    process.exitCode = 1;
+  } else {
+    log(green('SUCCESS:'), 'No invalid whitespaces found.');
+  }
+}
+
+/**
+ * Checks multiple kinds of files for invalid whitespaces.
+ */
+function checkInvalidWhitespaces() {
+  const filesToCheck = getFilesToCheck(invalidWhitespaceGlobs);
+  if (filesToCheck.length == 0) {
+    return;
+  }
+  runCheck(filesToCheck);
+}
+
+checkInvalidWhitespaces.description =
+  'Checks different types of files for invalid whitespaces.';
+checkInvalidWhitespaces.flags = {
+  'files': 'Checks just the specified files',
+  'local_changes': 'Checks just the files changed in the local branch',
+};
+
+module.exports = {
+  checkInvalidWhitespaces,
+};

build-system/tasks/pr-check.js

@@ -55,6 +55,10 @@ async function prCheck() {
     runCheck('amp presubmit');
   }
 
+  if (buildTargetsInclude(Targets.INVALID_WHITESPACES)) {
+    runCheck('amp check-invalid-whitespaces --local_changes');
+  }
+
   if (buildTargetsInclude(Targets.LINT)) {
     runCheck('amp lint --local_changes');
   }

build-system/test-configs/config.js

@@ -178,6 +178,21 @@ const linkCheckGlobs = [
   '!**/{examples,node_modules,build,dist,dist.3p,dist.tools}/**',
 ];
 
+/**
+ * List of files checked by `amp check-invalid-whitespaces`.
+ * - JS files are already checked by `amp lint` using eslint / prettier.
+ * - Markdown files are ignored because line-breaks use trailing whitespaces.
+ */
+const invalidWhitespaceGlobs = [
+  '**/*.css',
+  '**/*.html',
+  '**/*.out',
+  '**/*.out.cpponly',
+  '**/*.protoascii',
+  '**/*.sh',
+  '!**/{node_modules,build,dist,dist.3p,dist.tools}/**',
+];
+
 /**
  * Array of 3p bootstrap urls
  * Defined by the following object schema:
@@ -227,4 +242,5 @@ module.exports = {
   thirdPartyFrames,
   unitTestCrossBrowserPaths,
   unitTestPaths,
+  invalidWhitespaceGlobs,
 };

css/ampshared.css

@@ -539,13 +539,13 @@ amp-accordion {
   display: block !important;
 }
 
-/** 
+/**
  * This media query is used instead of @supports due to low compatibility.
  * At the time of this change (4/6/2021) no version of Webkit support @supports queries.
 
- * These provide low-specificity UI styles to prevent CLS for the header and content 
- * segments of accordions. The same styles are later set and extended with interactive 
- * styles for older browsers via the `i-amphtml-accordion-header` class in both 0.1 and 
+ * These provide low-specificity UI styles to prevent CLS for the header and content
+ * segments of accordions. The same styles are later set and extended with interactive
+ * styles for older browsers via the `i-amphtml-accordion-header` class in both 0.1 and
  * the 1.0 version of accordion.
  */
 @media (min-width: 1px) {

examples/amp-ad/doubleclick-targeting-macro.html

@@ -7,16 +7,16 @@
   <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
   <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
   <style amp-custom>
-    amp-ad { 
-        border: 1px solid #ccc; 
+    amp-ad {
+        border: 1px solid #ccc;
     }
   </style>
   <script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script>
   <script async src="https://cdn.ampproject.org/v0.js"></script>
 </head>
 <body>
     <h2>A4A ad</h2>
-    <amp-ad  height="250" 
+    <amp-ad  height="250"
         type="doubleclick"
         width="300"
         data-slot="/30497360/a4a/a4a_native"

examples/amp-consent/amp-consent-3p-postmessage.html

@@ -147,7 +147,7 @@ <h3>Image that is NOT blocked by consent</h3>
         This is a fallback
       </div>
     </amp-video-iframe>
-    
+
     <amp-video-iframe data-block-on-consent id="myVideo2" src="./diy-3p-iframe-tcf-postmessage.html" width="720" height="405" layout="responsive" autoplay>
       <div placeholder>
         This is a placeholder

examples/amp-consent/amp-consent-amp-ad.amp.html

@@ -63,4 +63,3 @@ <h2>Doubleclick with RTC (Blocked by _till_accepted)</h2>
 
 </body>
 </html>
-

examples/amp-consent/diy-3p-iframe-tcf-postmessage.html

@@ -141,9 +141,9 @@
         window.__tcfapi = function (cmd, version, callback, arg) {
           if (!cmpFrame) {
               callback(
-                { 
-                  msg: "CMP not found" 
-                }, 
+                {
+                  msg: "CMP not found"
+                },
                 false
               );
             } else {
@@ -206,12 +206,12 @@
         // should get response from window.top's CMP
         console.log('Ping return:', pingReturn);
       });
-      
+
       __tcfapi("getTCData", 2, (tcData, success) => {
         // should get response from window.top's CMP
         console.log('Minimal TCData return:', tcData);
       });
-      
+
       const callback = (tcData, success) => {
         console.log('addEventListenerCallback:', tcData)
         if(success && tcData.eventStatus === 'tcloaded') {

examples/amp-fit-text.amp.html

@@ -35,7 +35,7 @@
       </style>
   </head>
   <body>
-    <!-- 
+    <!--
       Basic usage
       In each of these examples, the `amp-fit-text` element is nested within a container with fixed dimensions to demonstrate the differences in attributes and layout values. When `amp-fit-text` is used with a given `height`, `width`, and `layout="responsive"` attribute, the text will scale to fit the parent container.
     -->
@@ -44,7 +44,7 @@
       <div class="square-medium"><amp-fit-text id="basic medium" width="1" height="1" layout="responsive">Lorem ipsum dolor sit amet, has nisl nihil convenire et, vim at aeque inermis reprehendunt.</amp-fit-text></div>
       <div class="square-large"><amp-fit-text  id="basic large" width="1" height="1" layout="responsive">Lorem ipsum dolor sit amet, has nisl nihil convenire et, vim at aeque inermis reprehendunt.</amp-fit-text></div>
     </div>
-    <!-- 
+    <!--
       Aspect ratio
       The text will scale responsively in the ratio defined by the responsive layout, but not exceed the size of its parent. This sample defines an aspect ratio of 1.5:1 (3:2).
     -->
@@ -53,7 +53,7 @@
       <div class="square-medium"><amp-fit-text id="aspect medium" width="300" height="200" layout="responsive">Lorem ipsum dolor sit amet, has nisl nihil convenire et, vim at aeque inermis reprehendunt.</amp-fit-text></div>
       <div class="square-large"><amp-fit-text id="aspect large" width="300" height="200" layout="responsive">Lorem ipsum dolor sit amet, has nisl nihil convenire et, vim at aeque inermis reprehendunt.</amp-fit-text></div>
     </div>
-    <!-- 
+    <!--
       min-font-size
       The `min-font-size` attribute dictates a minimum size for the text.  In this case, we've dictated a minimum of 30, which will cause it to exceed the size of its fixed block parent and be truncated to fit as appropriate.
     -->

examples/amp-geo.amp.html

@@ -19,13 +19,13 @@
       transform: rotate(180deg);
       transition: transform .5s .2s;
     }
-    
+
   </style>
   <script async custom-element="amp-geo" src="https://cdn.ampproject.org/v0/amp-geo-0.1.js"></script>
   <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
   <script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js"></script>
   <script async src="https://cdn.ampproject.org/v0.js"></script>
-  
+
 </head>
 <body>
   <amp-geo layout="nodisplay">
@@ -65,7 +65,7 @@ <h1>My article title</h1>
       append #amp-geo=&lt;code&gt; to the url and reload the page
       (eg #amp-geo=nz)
   </p>
-  
+
 
   <button on="tap:AMP.setState({myCountry: ampGeo.ISOCountry})">Where am I?</button>
   <p [text]="'The country code is: ' + myCountry "></p>

examples/amp-gist.amp.html

@@ -15,21 +15,21 @@
 <body>
   <h1>AMP-GIST Examples</h1>
   <h2>Long Example</h2>
-  <amp-gist 
-    layout="fixed-height" 
-    data-gistid="a19e811dcd7df10c4da0931641538497" 
+  <amp-gist
+    layout="fixed-height"
+    data-gistid="a19e811dcd7df10c4da0931641538497"
     height="1613">
   </amp-gist>
   <h2>Smaller Example</h2>
-  <amp-gist 
-    layout="fixed-height" 
-    data-gistid="b9bb35bc68df68259af94430f012425f" 
+  <amp-gist
+    layout="fixed-height"
+    data-gistid="b9bb35bc68df68259af94430f012425f"
     height="225">
   </amp-gist>
   <h2>Single File Example</h2>
-  <amp-gist 
-    layout="fixed-height" 
-    data-gistid="a19e811dcd7df10c4da0931641538497" 
+  <amp-gist
+    layout="fixed-height"
+    data-gistid="a19e811dcd7df10c4da0931641538497"
     data-file="index.js"
     height="65">
   </amp-gist>

examples/amp-layout-intrinsic.amp.html

@@ -12,18 +12,18 @@
     body {
       padding: 10px;
     }
-  
+
     .floated {
       max-width: 1000px;
       float: left;
       padding: 10px;
       background: #ccc;
     }
-  
+
     h2 {
       clear: both
     }
-  
+
     [fallback] {
       display: block;
       /* @alternative */
@@ -35,11 +35,11 @@
       background: rgba(0, 0, 0, 0.5);
       color: #fff;
     }
-  
+
     .spacer {
       height: 100vh;
     }
-  
+
     p,
     h1,
     h2,