fix(ie): Use 32-bit version by default for IEDriver #181

cnishina · 2016-12-22T23:48:00Z

closes #180

sjelin

A couple questions.

sjelin · 2016-12-27T19:12:21Z

lib/cmds/start.ts

@@ -153,7 +154,7 @@ function start(options: Options) {
        path.resolve(outputDir, binaries[GeckoDriver.id].executableFilename(osType)));
  }
  if (downloadedBinaries[IEDriver.id] != null) {
-    if (options[Opt.IE32].getBoolean()) {
+    if (options[Opt.IE32].getBoolean() || options[Opt.IE].getBoolean()) {


This conditional sets the architecture to 32-bit. By default it is the system's arch which is probably x64.

sjelin · 2016-12-27T19:13:22Z

lib/cmds/update.ts

    let binary = binaries[IEDriver.id];
    binary.arch = Config.osArch();  // Win32 or x64
    updateBrowserFile(binary, outputDir);
    promises.push(updateBinary(binary, outputDir, proxy, ignoreSSL));
  }
-  if (ie32) {
+  if (ie || ie32) {


if ((ie && !ie64) || ie32)?

ie, ie32, and ie64 are set from different flag options.

An example: a user downloads the 32bit version:

webdriver-manager update --ie ie = true ie32 = false ie64 = false webdriver-manager update --ie32 ie = false ie32 = true ie64 = false

An odd case: the user wants to download the 32bit version and 64bit version. We will let them download both. However, starting the server, the user would then have to specify if they want to use the 32-bit version if not it would default to the system arch.

webdriver-manager update --ie32 --ie64 ie = false ie32 = true ie64 = true

sjelin · 2016-12-27T19:14:43Z

lib/files/file_manager.ts

@@ -122,7 +122,9 @@ export class FileManager {
        }
        // if the suffix does not match the executable,
        // the binary is something like: .exe and .zip
-        else if (existFile.indexOf(binary.suffix(osType, arch)) === -1) {
+        else if (


Why is this change here?

This is an extra check... current implementation of this needs some fixing. This is because downloading a file before 2.23 suffix is mac32.zip and this method is returning mac64.zip. Unfortunately, when we get the suffix the version is not set.

Not having extra checks:

webdriver-manager status chromedriver versions available: 2.20 [last], 2.20mac32.zip, 2.26 [default]

Adding a TODO(cnishina) to fix this.

sjelin · 2016-12-27T19:14:53Z

spec/cmds/status_spec.ts

@@ -18,8 +18,6 @@ describe('status', () => {
  // geckodriver {{config version}} [last] [default]
  // standalone 2.24 [last], {{config version}} [default]
  beforeAll((done) => {
-    Config.osType_ = 'Linux';
-    Config.osArch_ = 'x64';


Why this change?

It does not matter which os arch and type we are downloading.

sjelin · 2016-12-27T19:16:10Z

lib/cmds/opts.ts

 opts[IE32] = new Option(IE32, 'Install or update 32-bit ie driver', 'boolean', IEDriver.isDefault);
+opts[IE64] = new Option(IE64, 'Install or update x64 iedriver', 'boolean', IEDriver.isDefault);


Shouldn't the default be IEDriver.is64Default?

isDefault is a boolean to either to download or not download. The arch version is decided elsewhere.

sjelin · 2016-12-28T19:18:54Z

lib/cmds/opts.ts

-opts[IE] = new Option(IE, 'Install or update ie driver', 'boolean', IEDriver.isDefault);
-opts[IE32] = new Option(IE32, 'Install or update 32-bit ie driver', 'boolean', IEDriver.isDefault);
+opts[IE] = new Option(
+    IE, 'Install or update ie driver (defaults downloading 32-bit version)', 'boolean',


Not a default - always 32 bit.

sjelin · 2016-12-28T19:23:54Z

lib/cmds/update.ts

@@ -74,12 +74,16 @@ function update(options: Options): Promise<void> {
  let gecko = options[Opt.GECKO].getBoolean();
  let ie: boolean = false;
  let ie32: boolean = false;


Why two variables if they're just gonna be the same thing anyway? Combine both into ie32

sjelin · 2016-12-28T19:36:16Z

lib/cmds/start.ts

@@ -46,6 +46,7 @@ if (Config.osType() === 'Windows_NT') {
  prog.addOption(Opts[Opt.VERSIONS_IE])
      .addOption(Opts[Opt.IE32])
      .addOption(Opts[Opt.IE])
+      .addOption(Opts[Opt.IE64])


As per our discussion offline, remove the IE and IE32 flags and default arch to Win32

cnishina · 2017-01-04T07:32:22Z

@sjelin ping... for review.

sjelin · 2017-01-04T19:38:07Z

lib/binaries/ie_driver.ts

@@ -10,7 +10,8 @@ export class IEDriver extends Binary {
  static os = [OS.Windows_NT];
  static id = 'ie';
  static versionDefault = Config.binaryVersions().ie;
-  static isDefault = false;
+  static isDefault32 = false;
+  static isDefault64 = false;
  static shortName = ['ie', 'ie32'];


Should the short names be ['ie', 'ie64'] now?

Would you imagine that. shortName is not used at all. I must have had it there for something and abandoned it. Removing all shortName's. 😕

sjelin

Oh actually see my shortName changes

sjelin · 2017-01-05T01:03:29Z

lib/binaries/binary.ts

@@ -21,7 +21,6 @@ export interface BinaryMap<T extends Binary> { [id: string]: T; }
 export class Binary {
  static os: OS[];                // the operating systems, the binary can run on
  static id: string;              // the binaries key identifier
-  static isDefault: boolean;      // to download by default
  static versionDefault: string;  // a static default version variable
  static shortName: string[];     // the names used for a binary download


If shortName is gone remove it here too. Please do a grep too do double check

Visual grep failed. Will fix.

Actually, it looks like I didn't commit that code... fixed now.

closes angular#180

googlebot added the cla: yes label Dec 22, 2016

cnishina requested a review from sjelin December 22, 2016 23:49

sjelin suggested changes Dec 27, 2016

View reviewed changes

cnishina force-pushed the fix_ie branch 2 times, most recently from 225cd9a to c9089da Compare December 27, 2016 22:05

cnishina added the pr: needs review label Dec 27, 2016

sjelin suggested changes Dec 28, 2016

View reviewed changes

cnishina force-pushed the fix_ie branch 2 times, most recently from d2065da to 9903d6c Compare December 28, 2016 19:45

sjelin approved these changes Jan 4, 2017

View reviewed changes

sjelin suggested changes Jan 4, 2017

View reviewed changes

cnishina force-pushed the fix_ie branch from 9903d6c to c05b6cc Compare January 4, 2017 22:59

sjelin suggested changes Jan 5, 2017

View reviewed changes

fix(ie): Use 32-bit version by default for IEDriver

8b48b95

closes angular#180

cnishina force-pushed the fix_ie branch from c05b6cc to 8b48b95 Compare January 5, 2017 06:42

sjelin approved these changes Jan 5, 2017

View reviewed changes

cnishina merged commit 70614a2 into angular:master Jan 5, 2017

cnishina deleted the fix_ie branch January 7, 2017 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ie): Use 32-bit version by default for IEDriver #181

fix(ie): Use 32-bit version by default for IEDriver #181

cnishina commented Dec 22, 2016

sjelin left a comment

sjelin Dec 27, 2016

cnishina Dec 27, 2016

sjelin Dec 27, 2016

cnishina Dec 27, 2016

sjelin Dec 27, 2016

cnishina Dec 27, 2016

sjelin Dec 27, 2016

cnishina Dec 27, 2016

sjelin Dec 27, 2016

cnishina Dec 27, 2016

sjelin Dec 28, 2016

sjelin Dec 28, 2016

sjelin Dec 28, 2016

cnishina commented Jan 4, 2017

sjelin Jan 4, 2017

cnishina Jan 4, 2017

sjelin left a comment

sjelin Jan 5, 2017

cnishina Jan 5, 2017

cnishina Jan 5, 2017

		opts[IE32] = new Option(IE32, 'Install or update 32-bit ie driver', 'boolean', IEDriver.isDefault);
		opts[IE64] = new Option(IE64, 'Install or update x64 iedriver', 'boolean', IEDriver.isDefault);

fix(ie): Use 32-bit version by default for IEDriver #181

fix(ie): Use 32-bit version by default for IEDriver #181

Conversation

cnishina commented Dec 22, 2016

sjelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnishina commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment