PR feedback

microsoft · Aug 25, 2016 · 3aef404 · 3aef404
1 parent 56b39ee
commit 3aef404
Show file tree

Hide file tree

Showing 9 changed files with 142 additions and 21 deletions.
diff --git a/Tasks/Common/nuget-task-common/NuGetQuirks.ts b/Tasks/Common/nuget-task-common/NuGetQuirks.ts
@@ -40,10 +40,10 @@ function closedRange(begin: VersionInfoVersion, end: VersionInfoVersion): Versio
 
 function versionIsInRange(version: VersionInfoVersion, range: VersionRange): boolean {
     const beginComparison = VersionInfoVersion.compare(version, range.begin);
-    const endComparison = VersionInfoVersion.compare(range.end, version);
+    const endComparison = VersionInfoVersion.compare(version, range.end);
 
     const beginResult = range.beginIsInclusive ? beginComparison >= 0 : beginComparison > 0;
-    const endResult = range.endIsInclusive ? endComparison >= 0 : endComparison > 0;
+    const endResult = range.endIsInclusive ? endComparison <= 0 : endComparison < 0;
 
     return beginResult && endResult;
 }
@@ -58,9 +58,9 @@ const nuget320 = new VersionInfoVersion(3, 2, 0, 0);
 const nuget330 = new VersionInfoVersion(3, 3, 0, 0);
 const nuget340 = new VersionInfoVersion(3, 4, 0, 0);
 const nuget350 = new VersionInfoVersion(3, 5, 0, 0);
-const nuget350_1707 = new VersionInfoVersion(3,5,0,1707)
+const nuget350_1707 = new VersionInfoVersion(3, 5, 0, 1707)
 const nuget351 = new VersionInfoVersion(3, 5, 1, 0);
-const nuget351_1707 = new VersionInfoVersion(3,5,1,1707)
+const nuget351_1707 = new VersionInfoVersion(3, 5, 1, 1707)
 
 const allQuirks: QuirkDescriptor[] = [
     {
@@ -86,7 +86,7 @@ const allQuirks: QuirkDescriptor[] = [
     },
     {
         quirk: NuGetQuirkName.NoV3,
-        versionRanges: [halfOpenRange(VersionInfoVersion.MIN_VERSION, nuget320)]
+        versionRanges: [halfOpenRange(VersionInfoVersion.MIN_VERSION, nuget300)]
     },
     {
         quirk: NuGetQuirkName.NoTfsOnPremAuthConfig,

diff --git a/Tasks/Common/nuget-task-common/pe-parser/PEImageFile.ts b/Tasks/Common/nuget-task-common/pe-parser/PEImageFile.ts
@@ -2,8 +2,9 @@ import IReadableFile from "./IReadableFile";
 import PEParserError from "./PEParserError";
 import {SectionTable, SectionTableEntry} from "./SectionTable";
 
-const peSignature = 0x00004550;
 const dosSignature = 0x5A4D;
+const fileOffsetOfPointerToPESignature = 0x3C;
+const peSignature = 0x00004550;
 
 const sizeOfCoffHeader = 20;
 
@@ -34,7 +35,7 @@ export class PEImageFile {
         }
 
         // read the pointer to the PE signature
-        await file.readAsync(buffer, 0, 4, 0x3c);
+        await file.readAsync(buffer, 0, 4, fileOffsetOfPointerToPESignature);
         const filePositionOfPEHeader = buffer.readUInt32LE(0);
 
         // read the PE signature

diff --git a/Tasks/Common/nuget-task-common/pe-parser/README.md b/Tasks/Common/nuget-task-common/pe-parser/README.md
@@ -0,0 +1,15 @@
+pe-parser
+=========
+
+Required reading:
+-----------------
+
+* [https://msdn.microsoft.com/en-us/library/windows/desktop/ms680547(v=vs.85).aspx](PE format documentation)
+* [https://msdn.microsoft.com/en-us/library/windows/desktop/ms647001(v=vs.85).aspx](Version resource documentation)
+
+If you're reading the code for the first time, I recommend skimming the docs above first, then start with getFileVersionInfoAsync in index.ts. The various .ts files
+mostly correspond to the major structures of the file:
+* PEImageFile => signatures and COFF header
+* SectionTable => section table
+* ResourceSection => resource table
+* VersionResource => VS\_VERSION\_INFO resource 
diff --git a/Tasks/Common/nuget-task-common/pe-parser/ResourceSection.ts b/Tasks/Common/nuget-task-common/pe-parser/ResourceSection.ts
@@ -48,17 +48,22 @@ export class ResourceDirectory {
     public entries: ResourceDirectoryEntry[] = [];
 
     public getDataEntry(id: string | number): ResourceData {
-        return <ResourceData>this.entries.find(x => x.id === id && !(x.data instanceof ResourceDirectory)).data;
+        const entry = this.entries.find(x => x.id === id && !(x.data instanceof ResourceDirectory));
+        if (!entry) {
+            return undefined;
+        }
+
+        return <ResourceData>entry.data;
     }
 
     public getSubdirectory(id: string | number): ResourceDirectory {
-        return <ResourceDirectory>this.entries.find(x => x.id === id && x.data instanceof ResourceDirectory).data;
-    }
-}
+        const entry = this.entries.find(x => x.id === id && x.data instanceof ResourceDirectory);
+        if (!entry) {
+            return undefined;
+        }
 
-interface CookingResourceDirectory {
-    rva: number;
-    directory: ResourceDirectory;
+        return <ResourceDirectory>entry.data;
+    }
 }
 
 async function readRawResourceDirectoryTableHeader(
@@ -140,8 +145,14 @@ async function readResourceDirectoryTable(
     file: IReadableFile,
     buffer: Buffer,
     resourceDirectoryFilePosition: number): Promise<ResourceDirectory> {
+
+    interface ResourceDirectoryQueueEntry {
+        rva: number;
+        directory: ResourceDirectory;
+    }
+
     let root: ResourceDirectory = new ResourceDirectory();
-    let queue: CookingResourceDirectory[] = [{ rva: 0, directory: root }];
+    let queue: ResourceDirectoryQueueEntry[] = [{ rva: 0, directory: root }];
 
     for (let current = queue.pop(); current !== undefined; current = queue.pop()) {
         let raw = await readRawResourceDirectoryTable(file, buffer, resourceDirectoryFilePosition + current.rva);

diff --git a/Tasks/Common/nuget-task-common/pe-parser/VersionResource.ts b/Tasks/Common/nuget-task-common/pe-parser/VersionResource.ts
@@ -1,3 +1,4 @@
+import PEParserError from "./PEParserError";
 import VersionInfoVersion from "./VersionInfoVersion";
 
 const neutralLanguageUnicodeCodePage = "000004b0";
@@ -72,7 +73,7 @@ function readNullTerminatedUcs2String(buffer: Buffer, offset: number): { value:
         end += 2;
     }
 
-    throw new Error("Unterminated string");
+    throw new PEParserError("unterminatedString", "Unterminated string");
 }
 
 function roundUp(value: number, multiple: number) {

diff --git a/Tasks/Common/nuget-task-common/tslint.json b/Tasks/Common/nuget-task-common/tslint.json
@@ -0,0 +1,31 @@
+{
+    "extends": [
+        "tslint:latest"
+    ],
+    "rules": {
+        "member-ordering": [ false ],
+        "interface-name": [ false ],
+
+        // lots of objects in this package are sorted by file order 
+        "object-literal-sort-keys": false,
+
+        "whitespace": [
+            "check-branch",
+            "check-decl",
+            "check-operator",
+            "check-module",
+            "check-separator",
+            "check-type"
+            /* "check-typecast" -- the vs code formatter introduces violations of these */
+        ],
+
+        "one-line": [
+            "check-open-brace",
+            "check-whitespace"
+            /* the vs code formatter introduces violations of these
+            "check-catch",
+            "check-finally",
+            "check-else" */
+        ]
+    }
+}
diff --git a/Tasks/NuGetInstaller/tslint.json b/Tasks/NuGetInstaller/tslint.json
@@ -0,0 +1,31 @@
+{
+    "extends": [
+        "tslint:latest"
+    ],
+    "rules": {
+        "member-ordering": [ false ],
+        "interface-name": [ false ],
+
+        // lots of objects in this package are sorted by file order 
+        "object-literal-sort-keys": false,
+
+        "whitespace": [
+            "check-branch",
+            "check-decl",
+            "check-operator",
+            "check-module",
+            "check-separator",
+            "check-type"
+            /* "check-typecast" -- the vs code formatter introduces violations of these */
+        ],
+
+        "one-line": [
+            "check-open-brace",
+            "check-whitespace"
+            /* the vs code formatter introduces violations of these
+            "check-catch",
+            "check-finally",
+            "check-else" */
+        ]
+    }
+}
diff --git a/Tasks/NugetPublisher/tslint.json b/Tasks/NugetPublisher/tslint.json
@@ -0,0 +1,31 @@
+{
+    "extends": [
+        "tslint:latest"
+    ],
+    "rules": {
+        "member-ordering": [ false ],
+        "interface-name": [ false ],
+
+        // lots of objects in this package are sorted by file order 
+        "object-literal-sort-keys": false,
+
+        "whitespace": [
+            "check-branch",
+            "check-decl",
+            "check-operator",
+            "check-module",
+            "check-separator",
+            "check-type"
+            /* "check-typecast" -- the vs code formatter introduces violations of these */
+        ],
+
+        "one-line": [
+            "check-open-brace",
+            "check-whitespace"
+            /* the vs code formatter introduces violations of these
+            "check-catch",
+            "check-finally",
+            "check-else" */
+        ]
+    }
+}
diff --git a/Tests/L0/Common-NuGetTaskCommon/_suite.ts b/Tests/L0/Common-NuGetTaskCommon/_suite.ts
@@ -109,20 +109,20 @@ describe("Common-NuGetTaskCommon Suite", () => {
                 ])
             },
             {
-                version: new VersionInfoVersion(3, 5, 0, 1737),
-                displayVersion: "3.5.0-rtm-1737",
+                version: new VersionInfoVersion(3, 5, 0, 1520),
+                displayVersion: "3.5.0-beta2-1520",
                 quirks: new Set([
                     NuGetQuirkName.NoTfsOnPremAuthConfig,
                     NuGetQuirkName.NoTfsOnPremAuthCredentialProvider,
+                    NuGetQuirkName.CredentialProviderRace,
                 ])
             },
             {
-                version: new VersionInfoVersion(3, 5, 0, 1520),
-                displayVersion: "3.5.0-beta2-1520",
+                version: new VersionInfoVersion(3, 5, 0, 1737),
+                displayVersion: "3.5.0-rtm-1737",
                 quirks: new Set([
                     NuGetQuirkName.NoTfsOnPremAuthConfig,
                     NuGetQuirkName.NoTfsOnPremAuthCredentialProvider,
-                    NuGetQuirkName.CredentialProviderRace,
                 ])
             },
             {