[PATCH setup 00/11] Various setup patches

[PATCH setup 00/11] Various setup patches

[Jon Turney]

More cleaning out of the Augean stables.

Jon Turney (11):
  Remove pointless abstract base class IniDBBuilder
  Remove unused package_status_t stored in packageversion class
  Remove cygpackage::destroy() because it does nothing
  Make packageversion::source(|s) const
  Use const version of packageversion::depends() in PrereqChecker
  packageversion::sourcePackageSpecification() is const
  Don't handle missing 'version:'
  Don't do unneeded work when changing stability level
  Make building with DEBUG less useless
  Don't show source-only packages in package list
  Use wininet for fetching URLs in direct (non-proxy) case (DO NOT
    APPLY)

 ConnectionSetting.cc   |   5 ++
 IniDBBuilder.h         |  59 --------------------
 IniDBBuilderPackage.cc |  15 -----
 IniDBBuilderPackage.h  |  62 ++++++++++++---------
 PickView.cc            |   7 ++-
 ScanFindVisitor.cc     |   4 +-
 ScanFindVisitor.h      |   6 +-
 choose.cc              |   8 ---
 cygpackage.cc          |  11 ----
 cygpackage.h           |   7 ---
 ini.h                  |   4 +-
 inilex.ll              |   4 +-
 iniparse.yy            |   4 +-
 net.cc                 |  34 ++++++------
 netio.cc               |  32 ++++++++++-
 netio.h                |   2 +
 nio-ie5.cc             |  23 +++++---
 nio-ie5.h              |   2 +-
 package_db.cc          |  12 ----
 package_db.h           |   1 -
 package_meta.cc        |  47 +++++-----------
 package_meta.h         |  13 ++---
 package_version.cc     | 147 +------------------------------------------------
 package_version.h      |  15 +----
 prereq.cc              |   4 +-
 res.rc                 |   2 +
 resource.h             |   1 +
 27 files changed, 148 insertions(+), 383 deletions(-)
 delete mode 100644 IniDBBuilder.h

-- 
2.12.2

[PATCH setup 02/11] Remove unused package_status_t stored in packageversion class

[Jon Turney]

The package installed/not installed status is tracked by the packagemeta
class, currently
---
 cygpackage.cc      | 3 ---
 cygpackage.h       | 6 ------
 package_db.cc      | 1 -
 package_version.cc | 1 -
 package_version.h  | 9 ---------
 5 files changed, 20 deletions(-)

diff --git a/cygpackage.cc b/cygpackage.cc
index cd4b6bd..14c0c98 100644
--- a/cygpackage.cc
+++ b/cygpackage.cc
@@ -36,7 +36,6 @@ packagev (),
 canonical (),
 sdesc (),
 ldesc (),
-status (package_installed),
 type (package_binary),
 listdata (),
 listfile ()
@@ -61,12 +60,10 @@ cygpackage::createInstance (const std::string& pkgname,
 packageversion
 cygpackage::createInstance (const std::string& pkgname,
                             const std::string& version,
-			    package_status_t const newstatus,
 			    package_type_t const newtype)
 {
   cygpackage *temp = new cygpackage;
   temp->name = pkgname;
-  temp->status = newstatus;
   temp->type = newtype;
   temp->setCanonicalVersion (version);
   return packageversion(temp);
diff --git a/cygpackage.h b/cygpackage.h
index 991072a..4c57b95 100644
--- a/cygpackage.h
+++ b/cygpackage.h
@@ -34,10 +34,6 @@ public:
   virtual const std::string Vendor_version ();
   virtual const std::string Package_version ();
   virtual const std::string Canonical_version ();
-  virtual package_status_t Status ()
-  {
-    return status;
-  };
   virtual package_type_t Type ()
   {
     return type;
@@ -72,7 +68,6 @@ public:
 
   static packageversion createInstance (const std::string& pkgname,
                                         const std::string& version,
-					package_status_t const,
 					package_type_t const);
 
 private:
@@ -86,7 +81,6 @@ private:
   char getfilenamebuffer[CYG_PATH_MAX];
 
 //  package_stability_t stability;
-  package_status_t status;
   package_type_t type;
 
   io_stream *listdata, *listfile;
diff --git a/package_db.cc b/package_db.cc
index ba1a561..a47fb11 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -107,7 +107,6 @@ packagedb::packagedb ()
 
 		  packageversion binary = 
 		    cygpackage::createInstance (pkgname, f.ver,
-						package_installed,
 						package_binary);
 
 		  pkg->add_version (binary);
diff --git a/package_version.cc b/package_version.cc
index 8ed8ac8..1a4d041 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -45,7 +45,6 @@ public:
   const std::string Package_version() {return std::string();}
   const std::string Canonical_version() {return std::string();}
   void setCanonicalVersion (const std::string& ) {}
-  package_status_t Status (){return package_notinstalled;}
   package_type_t Type () {return package_binary;}
   const std::string getfirstfile () {return std::string();}
   const std::string getnextfile () {return std::string();}
diff --git a/package_version.h b/package_version.h
index edfd15a..c271f73 100644
--- a/package_version.h
+++ b/package_version.h
@@ -57,13 +57,6 @@ package_stability_t;
 
 typedef enum
 {
-  package_notinstalled,
-  package_installed
-}
-package_status_t;
-
-typedef enum
-{
   package_binary,
   package_source
 }
@@ -105,7 +98,6 @@ public:
   const std::string Package_version () const;
   const std::string Canonical_version () const;
   void setCanonicalVersion (const std::string& );
-  package_status_t Status () const;
   package_type_t Type () const;
   const std::string getfirstfile ();
   const std::string getnextfile ();
@@ -165,7 +157,6 @@ public:
   virtual const std::string Package_version () = 0;
   virtual const std::string Canonical_version () = 0;
   virtual void setCanonicalVersion (const std::string& ) = 0;
-  virtual package_status_t Status () = 0;
 //  virtual package_stability_t Stability () = 0;
   virtual package_type_t Type () = 0;
   /* TODO: we should probably return a metaclass - file name & path & size & type
-- 
2.12.2

[PATCH setup 01/11] Remove pointless abstract base class IniDBBuilder

[Jon Turney]

There can be no instances of IniDBBuilder (it has pure virtual methods).
There is only one derived class IniDBBuilderPackage.
We can't think of any use for other derived classes.
---
 IniDBBuilder.h        | 59 ------------------------------------------------
 IniDBBuilderPackage.h | 62 ++++++++++++++++++++++++++++++---------------------
 ScanFindVisitor.cc    |  4 ++--
 ScanFindVisitor.h     |  6 ++---
 ini.h                 |  4 ++--
 inilex.ll             |  4 ++--
 iniparse.yy           |  4 ++--
 7 files changed, 47 insertions(+), 96 deletions(-)
 delete mode 100644 IniDBBuilder.h

diff --git a/IniDBBuilder.h b/IniDBBuilder.h
deleted file mode 100644
index b3f5c57..0000000
--- a/IniDBBuilder.h
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * Copyright (c) 2002, Robert Collins.
- *
- *     This program is free software; you can redistribute it and/or modify
- *     it under the terms of the GNU General Public License as published by
- *     the Free Software Foundation; either version 2 of the License, or
- *     (at your option) any later version.
- *
- *     A copy of the GNU General Public License can be found at
- *     http://www.gnu.org/
- *
- * Written by Robert Collins  <rbtcollins@hotmail.com>
- *
- */
-
-#ifndef SETUP_INIDBBUILDER_H
-#define SETUP_INIDBBUILDER_H
-
-#include "PackageSpecification.h"
-
-class IniDBBuilder
-{
-public:
-  virtual ~IniDBBuilder() {};
-  virtual void buildTimestamp (const std::string& ) = 0;
-  virtual void buildVersion (const std::string& ) = 0;
-  virtual void buildPackage (const std::string& ) = 0;
-  virtual void buildPackageVersion (const std::string& ) = 0;
-  virtual void buildPackageSDesc (const std::string& ) = 0;
-  virtual void buildPackageLDesc (const std::string& ) = 0;
-  virtual void buildPackageInstall (const std::string& ) = 0;
-  virtual void buildPackageSource (const std::string&, const std::string&) = 0;
-  virtual void buildPackageTrust (int) = 0;
-  virtual void buildPackageCategory (const std::string& ) = 0;
-  virtual void buildBeginDepends () = 0;
-  virtual void buildInstallSize (const std::string& ) = 0;
-  virtual void buildInstallSHA512 (unsigned char const[64]) = 0;
-  virtual void buildSourceSHA512 (unsigned char const[64]) = 0;
-  virtual void buildInstallMD5 (unsigned char const[16]) = 0;
-  virtual void buildSourceMD5 (unsigned char const[16]) = 0;
-  virtual void buildBeginBuildDepends () = 0;
-  virtual void buildSourceName (const std::string& ) = 0;
-  virtual void buildSourceNameVersion (const std::string& ) = 0;
-  virtual void buildPackageListAndNode () = 0;
-  virtual void buildPackageListOrNode (const std::string& ) = 0;
-  virtual void buildPackageListOperator (PackageSpecification::_operators const &) = 0;
-  virtual void buildPackageListOperatorVersion (const std::string& ) = 0;
-  virtual void buildMessage (const std::string&, const std::string&) = 0;
-  void set_arch (const std::string& a) { arch = a; }
-  void set_release (const std::string& rel) { release = rel; }
-
-  unsigned int timestamp;
-  std::string arch;
-  std::string release;
-  std::string version;
-  std::string parse_mirror;
-};
-
-#endif /* SETUP_INIDBBUILDER_H */
diff --git a/IniDBBuilderPackage.h b/IniDBBuilderPackage.h
index a97c53f..a39a95f 100644
--- a/IniDBBuilderPackage.h
+++ b/IniDBBuilderPackage.h
@@ -16,48 +16,58 @@
 #ifndef SETUP_INIDBBUILDERPACKAGE_H
 #define SETUP_INIDBBUILDERPACKAGE_H
 
-#include "IniDBBuilder.h"
 #include <vector>
 #include "package_version.h"
 class IniParseFeedback;
 class packagesource;
 class packagemeta;
 
-class IniDBBuilderPackage:public IniDBBuilder
+class IniDBBuilderPackage
 {
 public:
   IniDBBuilderPackage (IniParseFeedback const &);
   ~IniDBBuilderPackage ();
-  virtual void buildTimestamp (const std::string& );
-  virtual void buildVersion (const std::string& );
-  virtual void buildPackage (const std::string& );
-  virtual void buildPackageVersion (const std::string& );
-  virtual void buildPackageSDesc (const std::string& );
-  virtual void buildPackageLDesc (const std::string& );
-  virtual void buildPackageInstall (const std::string& );
-  virtual void buildPackageSource (const std::string&, const std::string&);
-  virtual void buildPackageTrust (int);
-  virtual void buildPackageCategory (const std::string& );
 
-  virtual void buildBeginDepends ();
-  virtual void buildInstallSize (const std::string& );
-  virtual void buildInstallSHA512 (unsigned char const[64]);
-  virtual void buildSourceSHA512 (unsigned char const[64]);
-  virtual void buildInstallMD5 (unsigned char const[16]);
-  virtual void buildSourceMD5 (unsigned char const[16]);
-  virtual void buildBeginBuildDepends ();
-  virtual void buildMessage (const std::string&, const std::string&);
-  virtual void buildSourceName (const std::string& );
-  virtual void buildSourceNameVersion (const std::string& );
-  virtual void buildPackageListAndNode ();
-  virtual void buildPackageListOrNode (const std::string& );
-  virtual void buildPackageListOperator (PackageSpecification::_operators const &);
-  virtual void buildPackageListOperatorVersion (const std::string& );
+  void buildTimestamp (const std::string& );
+  void buildVersion (const std::string& );
+  void buildPackage (const std::string& );
+  void buildPackageVersion (const std::string& );
+  void buildPackageSDesc (const std::string& );
+  void buildPackageLDesc (const std::string& );
+  void buildPackageInstall (const std::string& );
+  void buildPackageSource (const std::string&, const std::string&);
+  void buildPackageTrust (int);
+  void buildPackageCategory (const std::string& );
+
+  void buildBeginDepends ();
+  void buildInstallSize (const std::string& );
+  void buildInstallSHA512 (unsigned char const[64]);
+  void buildSourceSHA512 (unsigned char const[64]);
+  void buildInstallMD5 (unsigned char const[16]);
+  void buildSourceMD5 (unsigned char const[16]);
+  void buildBeginBuildDepends ();
+  void buildMessage (const std::string&, const std::string&);
+  void buildSourceName (const std::string& );
+  void buildSourceNameVersion (const std::string& );
+  void buildPackageListAndNode ();
+  void buildPackageListOrNode (const std::string& );
+  void buildPackageListOperator (PackageSpecification::_operators const &);
+  void buildPackageListOperatorVersion (const std::string& );
+
+  void set_arch (const std::string& a) { arch = a; }
+  void set_release (const std::string& rel) { release = rel; }
+
+  unsigned int timestamp;
+  std::string arch;
+  std::string release;
+  std::string version;
+  std::string parse_mirror;
 
 private:
   void add_correct_version();
   void process_src (packagesource &src, const std::string& );
   void setSourceSize (packagesource &src, const std::string& );
+
   packagemeta *cp;
   packageversion cbpv;
   packagemeta *csp;
diff --git a/ScanFindVisitor.cc b/ScanFindVisitor.cc
index 50e7415..02cd6e8 100644
--- a/ScanFindVisitor.cc
+++ b/ScanFindVisitor.cc
@@ -15,9 +15,9 @@
 
 #include "ScanFindVisitor.h"
 #include "filemanip.h"
-#include "IniDBBuilder.h"
+#include "IniDBBuilderPackage.h"
 
-ScanFindVisitor::ScanFindVisitor(IniDBBuilder &aBuilder) : _Builder (aBuilder) {}
+ScanFindVisitor::ScanFindVisitor(IniDBBuilderPackage &aBuilder) : _Builder (aBuilder) {}
 ScanFindVisitor::~ScanFindVisitor(){}
 
 /* look for potential packages we can add to the in-memory package
diff --git a/ScanFindVisitor.h b/ScanFindVisitor.h
index 5755ca4..b4c93d9 100644
--- a/ScanFindVisitor.h
+++ b/ScanFindVisitor.h
@@ -18,19 +18,19 @@
 
 #include "FindVisitor.h"
 
-class IniDBBuilder;
+class IniDBBuilderPackage ;
 /* Scan files and create a package db when no cached .ini exists */
 class ScanFindVisitor : public FindVisitor
 {
 public:
-  ScanFindVisitor (IniDBBuilder &aBuilder);
+  ScanFindVisitor (IniDBBuilderPackage &aBuilder);
   virtual void visitFile(const std::string& basePath, const WIN32_FIND_DATA *);
   virtual ~ ScanFindVisitor ();
 protected:
   ScanFindVisitor (ScanFindVisitor const &);
   ScanFindVisitor & operator= (ScanFindVisitor const &);
 private:
-  IniDBBuilder &_Builder;
+  IniDBBuilderPackage &_Builder;
 };
 
 #endif /* SETUP_SCANFINDVISITOR_H */
diff --git a/ini.h b/ini.h
index 3072637..4b9ed69 100644
--- a/ini.h
+++ b/ini.h
@@ -30,9 +30,9 @@ extern std::string SetupIniDir;
 extern std::string SetupBaseName;
 
 class IniState;
-class IniDBBuilder;
+class IniDBBuilderPackage;
 class IniParseFeedback;
-void ini_init (io_stream *, IniDBBuilder *, IniParseFeedback &);
+void ini_init (io_stream *, IniDBBuilderPackage *, IniParseFeedback &);
 #define YYSTYPE char *
 
 /* When setup.ini is parsed, the information is stored according to
diff --git a/inilex.ll b/inilex.ll
index a6ae1fb..798a04b 100644
--- a/inilex.ll
+++ b/inilex.ll
@@ -160,13 +160,13 @@ B64	[a-zA-Z0-9_-]
 #include "io_stream.h"
 
 static io_stream *input_stream = 0;
-extern IniDBBuilder *iniBuilder;
+extern IniDBBuilderPackage *iniBuilder;
 static IniParseFeedback *iniFeedback;
 std::string current_ini_name, yyerror_messages;
 int yyerror_count;
 
 void
-ini_init(io_stream *stream, IniDBBuilder *aBuilder, IniParseFeedback &aFeedback)
+ini_init(io_stream *stream, IniDBBuilderPackage *aBuilder, IniParseFeedback &aFeedback)
 {
   input_stream = stream;
   iniBuilder = aBuilder;
diff --git a/iniparse.yy b/iniparse.yy
index 6213b70..9ef4801 100644
--- a/iniparse.yy
+++ b/iniparse.yy
@@ -25,13 +25,13 @@
 extern int yyerror (const std::string& s);
 int yylex ();
 
-#include "IniDBBuilder.h"
+#include "IniDBBuilderPackage.h"
 
 #define YYERROR_VERBOSE 1
 #define YYINITDEPTH 1000
 /*#define YYDEBUG 1*/
 
-IniDBBuilder *iniBuilder;
+IniDBBuilderPackage *iniBuilder;
 extern int yylineno;
 %}
 
-- 
2.12.2

[PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

From the discussion in [1], I was somewhat surprised to learn that setup
doesn't support https or ftps.

Switch to using wininet for fetching URLs in the direct (non-proxy) case, as
well. (It's already used in proxy case). This allows https and ftps
protocols to be used.

For the moment, we keep around the existing, hand-built URL fetching as
'Direct (legacy)'.

Problems with this patch:

No progress feedback as we download.  We just get handed the whole file by
wininet.

Files are cached by wininet.  This is good for setup.ini, as we don't fetch
it again when it hasn't changed, but bad for package archives as we end up
with two copies (one in wininet's cache and one in setup's cache).

I think the reason we have a handbuilt HTTP client is that back in 2000 or
so, we were concerned about the case where IE5 wasn't installed and so
wininet wasn't available.  But who knows...

[1] https://cygwin.com/ml/cygwin/2017-03/msg00384.html
---
 ConnectionSetting.cc |  5 +++++
 net.cc               | 34 +++++++++++++++++-----------------
 netio.cc             | 32 +++++++++++++++++++++++++++++---
 netio.h              |  2 ++
 nio-ie5.cc           | 23 ++++++++++++++---------
 nio-ie5.h            |  2 +-
 res.rc               |  2 ++
 resource.h           |  1 +
 8 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/ConnectionSetting.cc b/ConnectionSetting.cc
index 5baf76c..1154d94 100644
--- a/ConnectionSetting.cc
+++ b/ConnectionSetting.cc
@@ -49,6 +49,9 @@ ConnectionSetting::~ConnectionSetting ()
       sprintf(port_str, "%d", NetIO::net_proxy_port);
       UserSettings::instance().set("net-proxy-port", port_str);
       break;
+    case IDC_NET_DIRECT_LEGACY:
+      UserSettings::instance().set("net-method", "Legacy");
+      break;
     default:
 	break;
     }
@@ -63,6 +66,8 @@ ConnectionSetting::typeFromString(const std::string& aType)
     return IDC_NET_IE5;
   if (!casecompare(aType, "Proxy"))
     return IDC_NET_PROXY;
+  if (!casecompare(aType, "Legacy"))
+    return IDC_NET_DIRECT_LEGACY;
 
   /* A sanish default */
   return IDC_NET_IE5;
diff --git a/net.cc b/net.cc
index 659cf9b..903f096 100644
--- a/net.cc
+++ b/net.cc
@@ -37,30 +37,31 @@ extern ThreeBarProgressPage Progress;
 
 static StringOption ProxyOption ("", 'p', "proxy", "HTTP/FTP proxy (host:port)", false);
 
-static int rb[] = { IDC_NET_IE5, IDC_NET_DIRECT, IDC_NET_PROXY, 0 };
+static int rb[] = { IDC_NET_IE5, IDC_NET_DIRECT, IDC_NET_PROXY, IDC_NET_DIRECT_LEGACY, 0 };
 static bool doing_loading = false;
 
 void
 NetPage::CheckIfEnableNext ()
 {
-  int e = 0, p = 0, pu = 0;
+  int e = 0, p = 0;
   DWORD ButtonFlags = PSWIZB_BACK;
 
-  if (NetIO::net_method == IDC_NET_IE5)
-    pu = 1;
-  if (NetIO::net_method == IDC_NET_IE5 || NetIO::net_method == IDC_NET_DIRECT)
+  if (NetIO::net_method == IDC_NET_IE5 ||
+      NetIO::net_method == IDC_NET_DIRECT ||
+      NetIO::net_method == IDC_NET_DIRECT_LEGACY)
     e = 1;
   else if (NetIO::net_method == IDC_NET_PROXY)
     {
-      p = pu = 1;
+      p = 1;
       if (NetIO::net_proxy_host && NetIO::net_proxy_port)
-	e = 1;
+        e = 1;
+    }
+
+  if (e)
+    {
+      // There's something in the proxy and port boxes, enable "Next".
+      ButtonFlags |= PSWIZB_NEXT;
     }
-	if (e)
-	{
-		// There's something in the proxy and port boxes, enable "Next".
-		ButtonFlags |= PSWIZB_NEXT;
-	}
 
   GetOwner ()->SetButtons (ButtonFlags);
 
@@ -131,8 +132,8 @@ NetPage::OnInit ()
 
   // Check to see if any radio buttons are selected. If not, select a default.
   if (SendMessage (GetDlgItem (IDC_NET_IE5), BM_GETCHECK, 0, 0) != BST_CHECKED
-      && SendMessage (GetDlgItem (IDC_NET_PROXY), BM_GETCHECK, 0, 0)
-	 != BST_CHECKED)
+      && SendMessage (GetDlgItem (IDC_NET_PROXY), BM_GETCHECK, 0, 0) != BST_CHECKED
+      && SendMessage (GetDlgItem (IDC_NET_DIRECT_LEGACY), BM_GETCHECK, 0, 0) != BST_CHECKED)
     SendMessage (GetDlgItem (IDC_NET_DIRECT), BM_CLICK, 0, 0);
 }
 
@@ -141,9 +142,7 @@ NetPage::OnNext ()
 {
   save_dialog (GetHWND ());
 
-  Log (LOG_PLAIN) << "net: "
-    << ((NetIO::net_method == IDC_NET_IE5) ? "IE5" :
-        (NetIO::net_method == IDC_NET_DIRECT) ? "Direct" : "Proxy") << endLog;
+  Log (LOG_PLAIN) << "net: " << NetIO::net_method_name()  << endLog;
 
   Progress.SetActivateTask (WM_APP_START_SITE_INFO_DOWNLOAD);
   return IDD_INSTATUS;
@@ -170,6 +169,7 @@ NetPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
     case IDC_NET_IE5:
     case IDC_NET_DIRECT:
     case IDC_NET_PROXY:
+    case IDC_NET_DIRECT_LEGACY:
     case IDC_PROXY_HOST:
     case IDC_PROXY_PORT:
       save_dialog (GetHWND());
diff --git a/netio.cc b/netio.cc
index 5ec0b9a..76d0c8a 100644
--- a/netio.cc
+++ b/netio.cc
@@ -126,22 +126,28 @@ NetIO::open (char const *url)
 {
   NetIO *rv = 0;
   enum
-  { http, ftp, file }
+  { http, https, ftp, ftps, file }
   proto;
   if (strncmp (url, "http://", 7) == 0)
     proto = http;
+  else if (strncmp (url, "https://", 8) == 0)
+    proto = https;
   else if (strncmp (url, "ftp://", 6) == 0)
     proto = ftp;
+  else if (strncmp (url, "ftps://", 7) == 0)
+    proto = ftps;
   else
     proto = file;
 
   if (proto == file)
     rv = new NetIO_File (url);
   else if (net_method == IDC_NET_IE5)
-    rv = new NetIO_IE5 (url);
+    rv = new NetIO_IE5 (url, false);
   else if (net_method == IDC_NET_PROXY)
     rv = new NetIO_HTTP (url);
   else if (net_method == IDC_NET_DIRECT)
+    rv = new NetIO_IE5 (url, true);
+  else if (net_method == IDC_NET_DIRECT_LEGACY)
     {
       switch (proto)
 	{
@@ -154,10 +160,12 @@ NetIO::open (char const *url)
 	case file:
 	  rv = new NetIO_File (url);
 	  break;
+	default:
+	  mbox (NULL, "Protocol not handled by legacy URL handler", "Cygwin Setup", MB_OK);
 	}
     }
 
-  if (!rv->ok ())
+  if (rv && !rv->ok ())
     {
       delete rv;
       return 0;
@@ -284,3 +292,21 @@ NetIO::get_ftp_auth (HWND owner)
   passwd = &net_ftp_passwd;
   return auth_common (hinstance, IDD_FTP_AUTH, owner);
 }
+
+const char *
+NetIO::net_method_name ()
+{
+  switch (net_method)
+    {
+    case IDC_NET_IE5:
+      return "IE5";
+    case IDC_NET_DIRECT:
+      return "Direct";
+    case IDC_NET_PROXY:
+      return "Proxy";
+    case IDC_NET_DIRECT_LEGACY:
+      return "Direct (legacy)";
+    default:
+      return "Unknown";
+    }
+}
diff --git a/netio.h b/netio.h
index aa06edb..ca2d55d 100644
--- a/netio.h
+++ b/netio.h
@@ -64,6 +64,8 @@ public:
   static char *net_proxy_host;
   static int net_proxy_port;
 
+  static const char *net_method_name();
+
   /* Helper functions for http/ftp protocols.  Both return nonzero for
      "cancel", zero for "ok".  They set net_proxy_user, etc, in
      state.h */
diff --git a/nio-ie5.cc b/nio-ie5.cc
index 236c459..5aab75b 100644
--- a/nio-ie5.cc
+++ b/nio-ie5.cc
@@ -27,28 +27,33 @@
 #include "netio.h"
 #include "nio-ie5.h"
 
-static HINTERNET internet = 0;
+static HINTERNET internet_direct = 0;
+static HINTERNET internet_preconfig = 0;
 
-NetIO_IE5::NetIO_IE5 (char const *_url):
+NetIO_IE5::NetIO_IE5 (char const *_url, bool direct):
 NetIO (_url)
 {
   int resend = 0;
+  HINTERNET *internet;
 
-  if (internet == 0)
+  if (direct)
+    internet = &internet_direct;
+  else
+    internet = &internet_preconfig;
+
+  if (*internet == 0)
     {
       InternetAttemptConnect (0);
-      internet = InternetOpen ("Cygwin Setup", INTERNET_OPEN_TYPE_PRECONFIG,
-			       NULL, NULL, 0);
+      *internet = InternetOpen ("Cygwin Setup",
+				direct ? INTERNET_OPEN_TYPE_DIRECT : INTERNET_OPEN_TYPE_PRECONFIG,
+				NULL, NULL, 0);
     }
 
   DWORD flags =
- //    INTERNET_FLAG_DONT_CACHE |
     INTERNET_FLAG_KEEP_CONNECTION |
- //   INTERNET_FLAG_PRAGMA_NOCACHE |
- //   INTERNET_FLAG_RELOAD |
     INTERNET_FLAG_EXISTING_CONNECT | INTERNET_FLAG_PASSIVE;
 
-  connection = InternetOpenUrl (internet, url, NULL, 0, flags, 0);
+  connection = InternetOpenUrl (*internet, url, NULL, 0, flags, 0);
 
 try_again:
 
diff --git a/nio-ie5.h b/nio-ie5.h
index 801cf8a..02a9b5b 100644
--- a/nio-ie5.h
+++ b/nio-ie5.h
@@ -22,7 +22,7 @@ class NetIO_IE5:public NetIO
 {
   HINTERNET connection;
 public:
-    NetIO_IE5 (char const *url);
+  NetIO_IE5 (char const *url, bool direct);
    ~NetIO_IE5 ();
   virtual int ok ();
   virtual int read (char *buf, int nbytes);
diff --git a/res.rc b/res.rc
index aad74ac..8f1ab10 100644
--- a/res.rc
+++ b/res.rc
@@ -163,6 +163,8 @@ BEGIN
                     BS_AUTORADIOBUTTON | WS_TABSTOP,60,70,185,10
     CONTROL         "Use HTTP/FTP &Proxy:",IDC_NET_PROXY,"Button",
                     BS_AUTORADIOBUTTON | WS_TABSTOP,60,85,88,10
+    CONTROL         "&Direct Connection (legacy)",IDC_NET_DIRECT_LEGACY,"Button",
+                    BS_AUTORADIOBUTTON | WS_TABSTOP,60,150,94,10
     EDITTEXT        IDC_PROXY_HOST,120,105,120,12,ES_AUTOHSCROLL | 
                     WS_DISABLED | WS_GROUP
     EDITTEXT        IDC_PROXY_PORT,120,125,30,12,ES_AUTOHSCROLL | 
diff --git a/resource.h b/resource.h
index 68e8023..5bbb668 100644
--- a/resource.h
+++ b/resource.h
@@ -175,3 +175,4 @@
 #define IDC_FILE_INUSE_EDIT               590
 #define IDC_FILE_INUSE_MSG                591
 #define IDC_FILE_INUSE_HELP               592
+#define IDC_NET_DIRECT_LEGACY             593
-- 
2.12.2

[PATCH setup 06/11] packageversion::sourcePackageSpecification() is const

[Jon Turney]

---
 package_version.cc | 2 +-
 package_version.h  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/package_version.cc b/package_version.cc
index 695641f..1f9f2d8 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -218,7 +218,7 @@ packageversion::sourcePackage() const
 }
 
 PackageSpecification &
-packageversion::sourcePackageSpecification ()
+packageversion::sourcePackageSpecification () const
 {
   return data->sourcePackageSpecification ();
 }
diff --git a/package_version.h b/package_version.h
index 6f6fcde..b728acc 100644
--- a/package_version.h
+++ b/package_version.h
@@ -106,7 +106,7 @@ public:
   const std::string LDesc () const;
   void set_ldesc (const std::string& );
   packageversion sourcePackage () const;
-  PackageSpecification & sourcePackageSpecification ();
+  PackageSpecification & sourcePackageSpecification () const;
   void setSourcePackageSpecification (PackageSpecification const &);
 
   /* invariant: these never return NULL */
-- 
2.12.2

[PATCH setup 09/11] Make building with DEBUG less useless

[Jon Turney]

Showing a messagebox with ldesc for every single package is annoying.
---
 IniDBBuilderPackage.cc | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index a453a7d..d506a9f 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -116,9 +116,6 @@ void
 IniDBBuilderPackage::buildPackageLDesc (const std::string& theDesc)
 {
   cbpv.set_ldesc(theDesc);
-#if DEBUG
-  _feedback.warning(theDesc.c_str());
-#endif
 }
 
 void
-- 
2.12.2

[PATCH setup 04/11] Make packageversion::source(|s) const

[Jon Turney]

Make packageversion::source() and sources() const

Remove a temporary used to dererence an packageversion iterator to avoid
problems with const-correctness
---
 PickView.cc        | 4 ++--
 package_version.cc | 4 ++--
 package_version.h  | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/PickView.cc b/PickView.cc
index 3de49f7..1ba55e8 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -442,9 +442,9 @@ PickView::init_headers (HDC dc)
           if (*i != pkg.installed)
             note_width (headers, dc, i->Canonical_version (),
                         HMARGIN + SPIN_WIDTH, new_col);
-	  std::string z = format_1000s(packageversion(*i).source ()->size);
+	  std::string z = format_1000s(i->source ()->size);
 	  note_width (headers, dc, z, HMARGIN, size_col);
-	  z = format_1000s(packageversion(i->sourcePackage ()).source ()->size);
+	  z = format_1000s(i->sourcePackage ().source ()->size);
 	  note_width (headers, dc, z, HMARGIN, size_col);
 	}
       std::string s = pkg.name;
diff --git a/package_version.cc b/package_version.cc
index 1a4d041..695641f 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -262,7 +262,7 @@ packageversion::uninstall ()
 }
 
 packagesource *
-packageversion::source ()
+packageversion::source () const
 {
   if (!data->sources.size())
     data->sources.push_back (packagesource());
@@ -270,7 +270,7 @@ packageversion::source ()
 }
 
 vector<packagesource> *
-packageversion::sources ()
+packageversion::sources () const
 {
   return &data->sources;
 }
diff --git a/package_version.h b/package_version.h
index c271f73..6f6fcde 100644
--- a/package_version.h
+++ b/package_version.h
@@ -118,9 +118,9 @@ public:
 
   void uninstall ();
   /* invariant: never null */
-  packagesource *source(); /* where can we source the file from */
+  packagesource *source() const; /* where can we source the file from */
   /* invariant: never null */
-  std::vector <packagesource> *sources(); /* expose the list of files.
+  std::vector <packagesource> *sources() const; /* expose the list of files.
 					source() returns the 'default' file
 					sources() allows managing multiple files
 					in a single package
-- 
2.12.2

[PATCH setup 07/11] Don't handle missing 'version:'

[Jon Turney]

If the setup.ini is missing a 'version:' line, parse the version number out
of the 'install:' filename.  Let's not do that anymore...
---
 IniDBBuilderPackage.cc | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index 582d249..a453a7d 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -24,7 +24,6 @@
 #include "package_meta.h"
 #include "package_version.h"
 #include "cygpackage.h"
-#include "filemanip.h"
 #include "ini.h"
 // for strtoul
 #include <string.h>
@@ -170,7 +169,6 @@ IniDBBuilderPackage::buildPackageSource (const std::string& path,
   spec.setOperator (PackageSpecification::Equals);
   spec.setVersion (cbpv.Canonical_version());
 
-  // process_src (*cspv.source(), path);
   setSourceSize (*cspv.source(), size);
 }
 
@@ -457,16 +455,6 @@ IniDBBuilderPackage::process_src (packagesource &src, const std::string& path)
   if (!src.Canonical())
     src.set_canonical (path.c_str());
   src.sites.push_back(site(parse_mirror));
-
-  if (!cbpv.Canonical_version ().size())
-    {
-      fileparse f;
-      if (parse_filename (path, f))
-	{
-	  cbpv.setCanonicalVersion (f.ver);
-	  add_correct_version ();
-	}
-    }
 }
 
 void
-- 
2.12.2

[PATCH setup 05/11] Use const version of packageversion::depends() in PrereqChecker

[Jon Turney]

---
 prereq.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/prereq.cc b/prereq.cc
index 0d3b93c..4b6cd68 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -211,10 +211,10 @@ PrereqChecker::isMet ()
 
       // Fetch the dependencies of the package. This assumes that the
       // dependencies of the prev, curr, and exp versions are all the same.
-      vector <vector <PackageSpecification *> *> *deps = pack->curr.depends ();
+      const vector <vector <PackageSpecification *> *> *deps = pack->curr.depends ();
 
       // go through the package's dependencies
-      for (vector <vector <PackageSpecification *> *>::iterator d =
+      for (vector <vector <PackageSpecification *> *>::const_iterator d =
             deps->begin (); d != deps->end (); ++d)
         {
           // XXX: the following assumes that there is only a single
-- 
2.12.2

[PATCH setup 08/11] Don't do unneeded work when changing stability level

[Jon Turney]

Since 2c4487b3, we stopped recomputing all the dependencies every time
something was changed in the PickView.  Remove all the depsolver code which
was used to do that.

The only remaining use was when we changed stability level, to select all
the package versions at that stability level.  That work is already being
done by packagedb::defaultTrust() (called by ChooserPage::changeTrust()).

(Note that this changes behaviour slightly: New dependencies of currently
installed packages used to be shown in the PickView, but now they won't be
reported until the PrereqChecker)

(This still leaves the crude depsolver we actually use in PrereqChecker,
which just selects all the unmet dependencies with the current trust level)
---
 choose.cc          |   8 ---
 package_db.cc      |  11 -----
 package_db.h       |   1 -
 package_meta.cc    |  35 +-------------
 package_meta.h     |  11 +----
 package_version.cc | 140 -----------------------------------------------------
 6 files changed, 3 insertions(+), 203 deletions(-)

diff --git a/choose.cc b/choose.cc
index a0dcc1b..2016caa 100644
--- a/choose.cc
+++ b/choose.cc
@@ -372,14 +372,6 @@ ChooserPage::changeTrust(trusts aTrust)
 {
   SetBusy ();
   chooser->defaultTrust (aTrust);
-  packagedb db;
-  db.markUnVisited ();
-
-  for (packagedb::packagecollection::iterator i = db.packages.begin(); i != db.packages.end(); i++)
-    {
-      i->second->set_requirements(aTrust);
-    }
-
   chooser->refresh();
   PrereqChecker p;
   p.setTrust (aTrust);
diff --git a/package_db.cc b/package_db.cc
index a47fb11..c2e1b63 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -381,17 +381,6 @@ packagedb::connectedEnd()
 }
 
 void
-packagedb::markUnVisited()
-{
-  for (packagedb::packagecollection::iterator n = packages.begin ();
-       n != packages.end (); ++n)
-    {
-      packagemeta & pkgm = *(n->second);
-      pkgm.visited(false);
-    }
-}
-
-void
 packagedb::setExistence ()
 {
   /* binary packages */
diff --git a/package_db.h b/package_db.h
index 6a99398..d02dbc4 100644
--- a/package_db.h
+++ b/package_db.h
@@ -72,7 +72,6 @@ public:
   PackageDBConnectedIterator connectedEnd();
   void fillMissingCategory();
   void defaultTrust (trusts trust);
-  void markUnVisited();
   void setExistence();
   typedef std::map <std::string, packagemeta *> packagecollection;
   /* all seen binary packages */
diff --git a/package_meta.cc b/package_meta.cc
index 4ea9a4f..0f97f1d 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -92,8 +92,7 @@ packagemeta::packagemeta (packagemeta const &rhs) :
   installed (rhs.installed), prev (rhs.prev),
   curr (rhs.curr),
   exp (rhs.exp),
-  desired (rhs.desired),
-  visited_(rhs.visited_)
+  desired (rhs.desired)
 {
   
 }
@@ -457,26 +456,6 @@ packagemeta::set_action (trusts const trust)
     user_picked = true;
 }
 
-int
-packagemeta::set_requirements (trusts deftrust, size_t depth)
-{
-  if (visited())
-    return 0;
-  /* Only prevent further checks once we have been required by something else */
-  if (depth)
-    visited(true);
-  int changed = 0;
-  /* handle build-depends */
-  if (depth == 0 && desired.sourcePackage ().picked())
-    changed += desired.sourcePackage ().set_requirements (deftrust, depth + 1);
-  if (!desired || (desired != installed && !desired.picked ()))
-    /* uninstall || source only */
-    return changed;
-
-  return changed + desired.set_requirements (deftrust, depth);
-}
-
-
 // Set a particular type of action.
 void
 packagemeta::set_action (_actions action, packageversion const &default_version)
@@ -612,18 +591,6 @@ packagemeta::trustLabel(packageversion const &aVersion) const
 }
 
 void
-packagemeta::visited(bool const &aBool)
-{
-  visited_ = aBool;
-}
-
-bool
-packagemeta::visited() const
-{
-  return visited_;
-}
-
-void
 packagemeta::logSelectionStatus() const
 {
   packagemeta const & pkg = *this;
diff --git a/package_meta.h b/package_meta.h
index f23073e..9bf8336 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -35,8 +35,7 @@ public:
   static void ScanDownloadedFiles (bool);
   packagemeta (packagemeta const &);
   packagemeta (const std::string& pkgname)
-  : name (pkgname), key(pkgname), user_picked (false),
-    visited_(false)
+  : name (pkgname), key(pkgname), user_picked (false)
   {
   }
 
@@ -44,8 +43,6 @@ public:
 
   void add_version (packageversion &);
   void set_installed (packageversion &);
-  void visited(bool const &);
-  bool visited() const;
   void addToCategoryBase();
   bool hasNoCategories() const;
   void setDefaultCategories();
@@ -74,10 +71,7 @@ public:
   void set_action (trusts const t);
   void set_action (_actions, packageversion const & default_version);
   void uninstall ();
-  int set_requirements (trusts deftrust, size_t depth);
-  // explicit separation for generic programming.
-  int set_requirements (trusts deftrust) 
-    { return set_requirements (deftrust, 0); }
+
   void set_message (const std::string& message_id, const std::string& message_string)
   {
     message.set (message_id, message_string);
@@ -155,7 +149,6 @@ protected:
   packagemeta &operator= (packagemeta const &);
 private:
   std::string trustLabel(packageversion const &) const;
-  bool visited_;
 };
 
 #endif /* SETUP_PACKAGE_META_H */
diff --git a/package_version.cc b/package_version.cc
index 1f9f2d8..7f52c27 100644
--- a/package_version.cc
+++ b/package_version.cc
@@ -309,146 +309,6 @@ packageversion::scan (bool mirror_mode)
     }
 }
 
-static bool
-checkForInstalled (PackageSpecification *spec)
-{
-  packagedb db;
-  packagemeta *required = db.findBinary (*spec);
-  if (!required)
-    return false;
-  if (spec->satisfies (required->installed)
-      && required->desired == required->installed )
-    /* done, found a satisfactory installed version that will remain
-       installed */
-    return true;
-  return false;
-}
-
-static bool
-checkForUpgradeable (PackageSpecification *spec)
-{
-  packagedb db;
-  packagemeta *required = db.findBinary (*spec);
-  if (!required || !required->installed)
-    return false;
-  for (set <packageversion>::iterator i = required->versions.begin();
-       i != required->versions.end(); ++i)
-    if (spec->satisfies (*i))
-      return true;
-  return false;
-}
-
-static bool
-checkForSatisfiable (PackageSpecification *spec)
-{
-  packagedb db;
-  packagemeta *required = db.findBinary (*spec);
-  if (!required)
-    return false;
-  for (set <packageversion>::iterator i = required->versions.begin();
-       i != required->versions.end(); ++i)
-    if (spec->satisfies (*i))
-      return true;
-  return false;
-}
-
-static int
-select (trusts deftrust, size_t depth, packagemeta *required,
-        const packageversion &aVersion)
-{
-  /* preserve source */
-  bool sourceticked = required->desired.sourcePackage ().picked();
-  /* install this version */
-  required->desired = aVersion;
-  required->desired.pick (required->installed != required->desired, required);
-  required->desired.sourcePackage ().pick (sourceticked, NULL);
-  /* does this requirement have requirements? */
-  return required->set_requirements (deftrust, depth + 1);
-}
-
-static int
-processOneDependency (trusts deftrust, size_t depth,
-                      PackageSpecification *spec)
-{
-  /* TODO: add this to a set of packages to be offered to meet the
-     requirement. For now, simply set the install to the first
-     satisfactory version. The user can step through the list if
-     desired */
-  packagedb db;
-  packagemeta *required = db.findBinary (*spec);
-
-  packageversion trusted = required->trustp(false, deftrust);
-  if (spec->satisfies (trusted)) {
-      return select (deftrust, depth, required, trusted);
-  }
-
-  Log (LOG_TIMESTAMP) << "Warning, the default trust level for package "
-    << trusted.Name() << " does not meet this specification " << *spec
-    << endLog;
-  
-  set <packageversion>::iterator v;
-  for (v = required->versions.begin();
-    v != required->versions.end() && !spec->satisfies (*v); ++v);
-
-  if (v == required->versions.end())
-      /* assert ?! */
-      return 0;
-  
-  return select (deftrust, depth, required, *v);
-}
-
-int
-packageversion::set_requirements (trusts deftrust, size_t depth)
-{
-  int changed = 0;
-  vector <vector <PackageSpecification *> *>::iterator dp = depends ()->begin();
-  /* cheap test for too much recursion */
-  if (depth > 30)
-    return changed;
-  /* walk through each and clause */
-  while (dp != depends ()->end())
-    {
-      /* three step:
-	 1) is a satisfactory or clause installed?
-	 2) is an unsatisfactory version of an or clause which has
-	 a satisfactory version available installed?
-	 3) is a satisfactory package available?
-	 */
-      /* check each or clause for an installed match */
-      vector <PackageSpecification *>::iterator i =
-	find_if ((*dp)->begin(), (*dp)->end(), checkForInstalled);
-      if (i != (*dp)->end())
-	{
-	  /* we found an installed ok package */
-	  /* next and clause */
-	  ++dp;
-	  continue;
-	}
-      /* check each or clause for an upgradeable version */
-      i = find_if ((*dp)->begin(), (*dp)->end(), checkForUpgradeable);
-      if (i != (*dp)->end())
-	{
-	  /* we found a package that can be up/downgraded to meet the
-	     requirement. (*i is the packagespec that can be satisfied.)
-	     */
-	  ++dp;
-	  changed += processOneDependency (deftrust, depth, *i) + 1;
-	  continue;
-	}
-      /* check each or clause for an installable version */
-      i = find_if ((*dp)->begin(), (*dp)->end(), checkForSatisfiable);
-      if (i != (*dp)->end())
-	{
-	  /* we found a package that can be installed to meet the requirement */
-	  ++dp;
-	  changed += processOneDependency (deftrust, depth, *i) + 1;
-	  continue;
-	}
-      ++dp;
-    }
-  return changed;
-}
-
 void
 packageversion::addScript(Script const &aScript)
 {
-- 
2.12.2

[PATCH setup 10/11] Don't show source-only packages in package list

[Jon Turney]

external-source: packages which have no binary package will be marked skip:
and so won't show up in setup.ini at all.

If we change to using the Source: line to identify a source package, which
is (by definition) source only, these will appear in setup.ini, but without
any install: lines, so we need to explcitly avoid showing them in the
package list.

Future work: perhaps add another view to show just source packages?
---
 PickView.cc     |  3 +++
 package_meta.cc | 12 ++++++++++++
 package_meta.h  |  2 ++
 3 files changed, 17 insertions(+)

diff --git a/PickView.cc b/PickView.cc
index 1ba55e8..b3b1c1a 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -165,6 +165,9 @@ PickView::setViewMode (views mode)
         {
           packagemeta & pkg = *(i->second);
 
+          if (!pkg.isBinary())
+            continue;
+
           if ( // "Full" : everything
               (view_mode == PickView::views::PackageFull)
 
diff --git a/package_meta.cc b/package_meta.cc
index 0f97f1d..24ed17e 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -537,6 +537,18 @@ packagemeta::sourceAccessible () const
       if (bin.sourcePackage().accessible())
         return true;
     }
+
+  return false;
+}
+
+bool
+packagemeta::isBinary () const
+{
+  for (set<packageversion>::iterator i=versions.begin();
+       i != versions.end(); ++i)
+    if ((i->Type() == package_binary) && i->accessible())
+      return true;
+
   return false;
 }
 
diff --git a/package_meta.h b/package_meta.h
index 9bf8336..5f7842c 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -142,6 +142,8 @@ public:
   bool accessible () const;
   bool sourceAccessible() const;
 
+  bool isBinary() const;
+
   void logSelectionStatus() const;
   void logAllVersions() const;
 
-- 
2.12.2

[PATCH setup 03/11] Remove cygpackage::destroy() because it does nothing

[Jon Turney]

---
 cygpackage.cc | 8 --------
 cygpackage.h  | 1 -
 2 files changed, 9 deletions(-)

diff --git a/cygpackage.cc b/cygpackage.cc
index 14c0c98..56c1da8 100644
--- a/cygpackage.cc
+++ b/cygpackage.cc
@@ -101,14 +101,6 @@ cygpackage::setCanonicalVersion (const std::string& version)
 
 cygpackage::~cygpackage ()
 {
-  destroy ();
-}
-
-/* helper functions */
-
-void
-cygpackage::destroy ()
-{
 }
 
 const std::string
diff --git a/cygpackage.h b/cygpackage.h
index 4c57b95..4022472 100644
--- a/cygpackage.h
+++ b/cygpackage.h
@@ -72,7 +72,6 @@ public:
 
 private:
   cygpackage ();
-  void destroy ();
   std::string name;
   std::string vendor;
   std::string packagev;
-- 
2.12.2

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Åke Rehnman]

Hi,

On 2017-04-28 14:12, Jon Turney wrote:
>  From the discussion in [1], I was somewhat surprised to learn that setup
> doesn't support https or ftps.
For the same exact reason I've just recently patched in curl....

> Problems with this patch:
>
> No progress feedback as we download.  We just get handed the whole file by
> wininet.
curl doesn't have this problem.

Although it swell a bit compare to the original installer, from 861kB to 
3766kB.

/Ake

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 28/04/2017 16:33, Ã…ke Rehnman wrote:
> On 2017-04-28 14:12, Jon Turney wrote:
>>  From the discussion in [1], I was somewhat surprised to learn that setup
>> doesn't support https or ftps.
> For the same exact reason I've just recently patched in curl....

I'd be very interested to see that patch :)

>> Problems with this patch:
>>
>> No progress feedback as we download.  We just get handed the whole
>> file by
>> wininet.
> curl doesn't have this problem.
>
> Although it swell a bit compare to the original installer, from 861kB to
> 3766kB.

I don't think that's a problem.

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On 29/04/2017 11:53, Jon Turney wrote:
> On 28/04/2017 16:33, Ã…ke Rehnman wrote:
>> On 2017-04-28 14:12, Jon Turney wrote:
>>> From the discussion in [1], I was somewhat surprised to learn
>>> that setup doesn't support https or ftps.
>> For the same exact reason I've just recently patched in curl....
>
> I'd be very interested to see that patch :)

Thanks very much for the patch.

So... I'm in two minds about this.   Your patch seems good, but perhaps 
the problems with using wininet aren't insurmountable, and avoiding 
another dependency would also be good.

Anyhow, I had another go the wininet patch, attached.  I guess you have 
some use case for this, so perhaps you could give it a try and see if it 
works for you?

[-- Attachment #2: 0001-Use-wininet-for-fetching-URLs-in-direct-non-proxy-ca.patch --]
[-- Type: text/plain, Size: 11637 bytes --]

From b11932cacaa1743b3dc60c149f268d767e99bf9f Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Thu, 30 Mar 2017 11:48:44 +0100
Subject: [PATCH setup] Use wininet for fetching URLs in direct (non-proxy)
 case

From the discussion in [1], I was somewhat surprised to learn that setup
doesn't support https or ftps.

Switch to using wininet for fetching URLs in the direct (non-proxy) case, as
well. (It's already used in proxy case). This allows https and ftps
protocols to be used.

For the moment, we keep around the existing, hand-built URL fetching as
'Direct (legacy)'.

Arrange for mirrors.lst and setup.ini to be cached by wininet, but not
package archives (as setup maintains it's own cache of those)

Read from wininet in chunks rather than the whole file, so we can report
progress feedback as we download.

Also fix up some bad indentation.

I think the reason we have a handbuilt HTTP client is that back in 2000 or
so, we were concerned about the case where IE5 wasn't installed and so
wininet wasn't available.  But who knows...

[1] https://cygwin.com/ml/cygwin/2017-03/msg00384.html

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 ConnectionSetting.cc |  5 +++++
 geturl.cc            |  8 ++++----
 net.cc               | 34 +++++++++++++++++-----------------
 netio.cc             | 34 ++++++++++++++++++++++++++++++----
 netio.h              |  4 +++-
 nio-ie5.cc           | 33 ++++++++++++++++++++++++---------
 nio-ie5.h            |  4 ++--
 res.rc               |  2 ++
 resource.h           |  1 +
 9 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/ConnectionSetting.cc b/ConnectionSetting.cc
index 5baf76c..1154d94 100644
--- a/ConnectionSetting.cc
+++ b/ConnectionSetting.cc
@@ -49,6 +49,9 @@ ConnectionSetting::~ConnectionSetting ()
       sprintf(port_str, "%d", NetIO::net_proxy_port);
       UserSettings::instance().set("net-proxy-port", port_str);
       break;
+    case IDC_NET_DIRECT_LEGACY:
+      UserSettings::instance().set("net-method", "Legacy");
+      break;
     default:
 	break;
     }
@@ -63,6 +66,8 @@ ConnectionSetting::typeFromString(const std::string& aType)
     return IDC_NET_IE5;
   if (!casecompare(aType, "Proxy"))
     return IDC_NET_PROXY;
+  if (!casecompare(aType, "Legacy"))
+    return IDC_NET_DIRECT_LEGACY;
 
   /* A sanish default */
   return IDC_NET_IE5;
diff --git a/geturl.cc b/geturl.cc
index 17ad8e9..5abd39a 100644
--- a/geturl.cc
+++ b/geturl.cc
@@ -106,12 +106,12 @@ progress (int bytes)
 }
 
 static void
-getUrlToStream (const string &_url, io_stream *output)
+getUrlToStream (const string &_url, io_stream *output, bool cachable)
 {
   Log (LOG_BABBLE) << "getUrlToStream " << _url << endLog;
   is_local_install = (source == IDC_SOURCE_LOCALDIR);
   init_dialog (_url, 0);
-  NetIO *n = NetIO::open (_url.c_str());
+  NetIO *n = NetIO::open (_url.c_str(), cachable);
   if (!n || !n->ok ())
     {
       delete n;
@@ -153,7 +153,7 @@ get_url_to_membuf (const string &_url, HWND owner)
   try 
     {
       Log (LOG_BABBLE) << "get_url_to_membuf " << _url << endLog;
-      getUrlToStream (_url, membuf);
+      getUrlToStream (_url, membuf, true);
       
       if (membuf->seek (0, IO_SEEK_SET))
     	{
@@ -213,7 +213,7 @@ get_url_to_file (const string &_url,
 
   remove (_filename.c_str());		/* but ignore errors */
 
-  NetIO *n = NetIO::open (_url.c_str());
+  NetIO *n = NetIO::open (_url.c_str(), false);
   if (!n || !n->ok ())
     {
       delete n;
diff --git a/net.cc b/net.cc
index 659cf9b..903f096 100644
--- a/net.cc
+++ b/net.cc
@@ -37,30 +37,31 @@ extern ThreeBarProgressPage Progress;
 
 static StringOption ProxyOption ("", 'p', "proxy", "HTTP/FTP proxy (host:port)", false);
 
-static int rb[] = { IDC_NET_IE5, IDC_NET_DIRECT, IDC_NET_PROXY, 0 };
+static int rb[] = { IDC_NET_IE5, IDC_NET_DIRECT, IDC_NET_PROXY, IDC_NET_DIRECT_LEGACY, 0 };
 static bool doing_loading = false;
 
 void
 NetPage::CheckIfEnableNext ()
 {
-  int e = 0, p = 0, pu = 0;
+  int e = 0, p = 0;
   DWORD ButtonFlags = PSWIZB_BACK;
 
-  if (NetIO::net_method == IDC_NET_IE5)
-    pu = 1;
-  if (NetIO::net_method == IDC_NET_IE5 || NetIO::net_method == IDC_NET_DIRECT)
+  if (NetIO::net_method == IDC_NET_IE5 ||
+      NetIO::net_method == IDC_NET_DIRECT ||
+      NetIO::net_method == IDC_NET_DIRECT_LEGACY)
     e = 1;
   else if (NetIO::net_method == IDC_NET_PROXY)
     {
-      p = pu = 1;
+      p = 1;
       if (NetIO::net_proxy_host && NetIO::net_proxy_port)
-	e = 1;
+        e = 1;
+    }
+
+  if (e)
+    {
+      // There's something in the proxy and port boxes, enable "Next".
+      ButtonFlags |= PSWIZB_NEXT;
     }
-	if (e)
-	{
-		// There's something in the proxy and port boxes, enable "Next".
-		ButtonFlags |= PSWIZB_NEXT;
-	}
 
   GetOwner ()->SetButtons (ButtonFlags);
 
@@ -131,8 +132,8 @@ NetPage::OnInit ()
 
   // Check to see if any radio buttons are selected. If not, select a default.
   if (SendMessage (GetDlgItem (IDC_NET_IE5), BM_GETCHECK, 0, 0) != BST_CHECKED
-      && SendMessage (GetDlgItem (IDC_NET_PROXY), BM_GETCHECK, 0, 0)
-	 != BST_CHECKED)
+      && SendMessage (GetDlgItem (IDC_NET_PROXY), BM_GETCHECK, 0, 0) != BST_CHECKED
+      && SendMessage (GetDlgItem (IDC_NET_DIRECT_LEGACY), BM_GETCHECK, 0, 0) != BST_CHECKED)
     SendMessage (GetDlgItem (IDC_NET_DIRECT), BM_CLICK, 0, 0);
 }
 
@@ -141,9 +142,7 @@ NetPage::OnNext ()
 {
   save_dialog (GetHWND ());
 
-  Log (LOG_PLAIN) << "net: "
-    << ((NetIO::net_method == IDC_NET_IE5) ? "IE5" :
-        (NetIO::net_method == IDC_NET_DIRECT) ? "Direct" : "Proxy") << endLog;
+  Log (LOG_PLAIN) << "net: " << NetIO::net_method_name()  << endLog;
 
   Progress.SetActivateTask (WM_APP_START_SITE_INFO_DOWNLOAD);
   return IDD_INSTATUS;
@@ -170,6 +169,7 @@ NetPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
     case IDC_NET_IE5:
     case IDC_NET_DIRECT:
     case IDC_NET_PROXY:
+    case IDC_NET_DIRECT_LEGACY:
     case IDC_PROXY_HOST:
     case IDC_PROXY_PORT:
       save_dialog (GetHWND());
diff --git a/netio.cc b/netio.cc
index 5ec0b9a..cf634c1 100644
--- a/netio.cc
+++ b/netio.cc
@@ -122,26 +122,32 @@ NetIO::read (char *buf, int nbytes)
 }
 
 NetIO *
-NetIO::open (char const *url)
+NetIO::open (char const *url, bool cachable)
 {
   NetIO *rv = 0;
   enum
-  { http, ftp, file }
+  { http, https, ftp, ftps, file }
   proto;
   if (strncmp (url, "http://", 7) == 0)
     proto = http;
+  else if (strncmp (url, "https://", 8) == 0)
+    proto = https;
   else if (strncmp (url, "ftp://", 6) == 0)
     proto = ftp;
+  else if (strncmp (url, "ftps://", 7) == 0)
+    proto = ftps;
   else
     proto = file;
 
   if (proto == file)
     rv = new NetIO_File (url);
   else if (net_method == IDC_NET_IE5)
-    rv = new NetIO_IE5 (url);
+    rv = new NetIO_IE5 (url, false, cachable);
   else if (net_method == IDC_NET_PROXY)
     rv = new NetIO_HTTP (url);
   else if (net_method == IDC_NET_DIRECT)
+    rv = new NetIO_IE5 (url, true, cachable);
+  else if (net_method == IDC_NET_DIRECT_LEGACY)
     {
       switch (proto)
 	{
@@ -154,10 +160,12 @@ NetIO::open (char const *url)
 	case file:
 	  rv = new NetIO_File (url);
 	  break;
+	default:
+	  mbox (NULL, "Protocol not handled by legacy URL handler", "Cygwin Setup", MB_OK);
 	}
     }
 
-  if (!rv->ok ())
+  if (rv && !rv->ok ())
     {
       delete rv;
       return 0;
@@ -284,3 +292,21 @@ NetIO::get_ftp_auth (HWND owner)
   passwd = &net_ftp_passwd;
   return auth_common (hinstance, IDD_FTP_AUTH, owner);
 }
+
+const char *
+NetIO::net_method_name ()
+{
+  switch (net_method)
+    {
+    case IDC_NET_IE5:
+      return "IE5";
+    case IDC_NET_DIRECT:
+      return "Direct";
+    case IDC_NET_PROXY:
+      return "Proxy";
+    case IDC_NET_DIRECT_LEGACY:
+      return "Direct (legacy)";
+    default:
+      return "Unknown";
+    }
+}
diff --git a/netio.h b/netio.h
index aa06edb..7b7d13f 100644
--- a/netio.h
+++ b/netio.h
@@ -51,7 +51,7 @@ public:
      the given URL.  It uses the network setup state in state.h.  If
      anything fails, either the return values is NULL or the returned
      object is !ok() */
-  static NetIO *open (char const *url);
+  static NetIO *open (char const *url, bool cachable);
 
   /* If !ok() that means the transfer isn't happening. */
   virtual int ok ();
@@ -64,6 +64,8 @@ public:
   static char *net_proxy_host;
   static int net_proxy_port;
 
+  static const char *net_method_name();
+
   /* Helper functions for http/ftp protocols.  Both return nonzero for
      "cancel", zero for "ok".  They set net_proxy_user, etc, in
      state.h */
diff --git a/nio-ie5.cc b/nio-ie5.cc
index 236c459..3375c04 100644
--- a/nio-ie5.cc
+++ b/nio-ie5.cc
@@ -27,28 +27,37 @@
 #include "netio.h"
 #include "nio-ie5.h"
 
-static HINTERNET internet = 0;
+static HINTERNET internet_direct = 0;
+static HINTERNET internet_preconfig = 0;
 
-NetIO_IE5::NetIO_IE5 (char const *_url):
+NetIO_IE5::NetIO_IE5 (char const *_url, bool direct, bool cachable):
 NetIO (_url)
 {
   int resend = 0;
+  HINTERNET *internet;
 
-  if (internet == 0)
+  if (direct)
+    internet = &internet_direct;
+  else
+    internet = &internet_preconfig;
+
+  if (*internet == 0)
     {
       InternetAttemptConnect (0);
-      internet = InternetOpen ("Cygwin Setup", INTERNET_OPEN_TYPE_PRECONFIG,
-			       NULL, NULL, 0);
+      *internet = InternetOpen ("Cygwin Setup",
+				direct ? INTERNET_OPEN_TYPE_DIRECT : INTERNET_OPEN_TYPE_PRECONFIG,
+				NULL, NULL, 0);
     }
 
   DWORD flags =
- //    INTERNET_FLAG_DONT_CACHE |
     INTERNET_FLAG_KEEP_CONNECTION |
- //   INTERNET_FLAG_PRAGMA_NOCACHE |
- //   INTERNET_FLAG_RELOAD |
     INTERNET_FLAG_EXISTING_CONNECT | INTERNET_FLAG_PASSIVE;
 
-  connection = InternetOpenUrl (internet, url, NULL, 0, flags, 0);
+  if (!cachable) {
+    flags |= INTERNET_FLAG_NO_CACHE_WRITE;
+  }
+
+  connection = InternetOpenUrl (*internet, url, NULL, 0, flags, 0);
 
 try_again:
 
@@ -147,8 +156,14 @@ NetIO_IE5::ok ()
 int
 NetIO_IE5::read (char *buf, int nbytes)
 {
+  /* Read in chunks rather than the whole file at once, so we can do progress
+     reporting */
+  if (nbytes > 4096)
+    nbytes = 4096;
+
   DWORD actual;
   if (InternetReadFile (connection, buf, nbytes, &actual))
     return actual;
+
   return -1;
 }
diff --git a/nio-ie5.h b/nio-ie5.h
index 801cf8a..9a66e2a 100644
--- a/nio-ie5.h
+++ b/nio-ie5.h
@@ -22,8 +22,8 @@ class NetIO_IE5:public NetIO
 {
   HINTERNET connection;
 public:
-    NetIO_IE5 (char const *url);
-   ~NetIO_IE5 ();
+  NetIO_IE5 (char const *url, bool direct, bool cacheable);
+  ~NetIO_IE5 ();
   virtual int ok ();
   virtual int read (char *buf, int nbytes);
   void flush_io ();
diff --git a/res.rc b/res.rc
index aad74ac..8f1ab10 100644
--- a/res.rc
+++ b/res.rc
@@ -163,6 +163,8 @@ BEGIN
                     BS_AUTORADIOBUTTON | WS_TABSTOP,60,70,185,10
     CONTROL         "Use HTTP/FTP &Proxy:",IDC_NET_PROXY,"Button",
                     BS_AUTORADIOBUTTON | WS_TABSTOP,60,85,88,10
+    CONTROL         "&Direct Connection (legacy)",IDC_NET_DIRECT_LEGACY,"Button",
+                    BS_AUTORADIOBUTTON | WS_TABSTOP,60,150,94,10
     EDITTEXT        IDC_PROXY_HOST,120,105,120,12,ES_AUTOHSCROLL | 
                     WS_DISABLED | WS_GROUP
     EDITTEXT        IDC_PROXY_PORT,120,125,30,12,ES_AUTOHSCROLL | 
diff --git a/resource.h b/resource.h
index 68e8023..5bbb668 100644
--- a/resource.h
+++ b/resource.h
@@ -175,3 +175,4 @@
 #define IDC_FILE_INUSE_EDIT               590
 #define IDC_FILE_INUSE_MSG                591
 #define IDC_FILE_INUSE_HELP               592
+#define IDC_NET_DIRECT_LEGACY             593
-- 
2.12.2

Re: [PATCH setup 10/11] Don't show source-only packages in package list

[Yaakov Selkowitz]

On 2017-04-28 07:12, Jon Turney wrote:
> external-source: packages which have no binary package will be marked skip:
> and so won't show up in setup.ini at all.
>
> If we change to using the Source: line to identify a source package, which
> is (by definition) source only, these will appear in setup.ini, but without
> any install: lines, so we need to explcitly avoid showing them in the
> package list.

Does this necessitate a change to cygport?

-- 
Yaakov

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Åke Rehnman]

Hello,

I tested with my repo and it seem to fail if there is no setup.xz but 
only setup.ini. It does quickly realize there are no setup.xz and 
setup.xz.sig but then it takes forever to get past setup.bz2 etc and 
fails to download setup.ini

These tests were done with -X flag.

I will email the link to the repo in a separate email.

/Ake


On 2017-05-01 14:58, Jon Turney wrote:
>
> Anyhow, I had another go the wininet patch, attached.  I guess you 
> have some use case for this, so perhaps you could give it a try and 
> see if it works for you?

Re: [PATCH setup 10/11] Don't show source-only packages in package list

[Jon Turney]

On 01/05/2017 15:45, Yaakov Selkowitz wrote:
> On 2017-04-28 07:12, Jon Turney wrote:
>> external-source: packages which have no binary package will be
>> marked skip: and so won't show up in setup.ini at all.
>>
>> If we change to using the Source: line to identify a source
>> package, which is (by definition) source only, these will appear in
>> setup.ini, but without any install: lines, so we need to explicitly
>> avoid showing them in the package list.
>
> Does this necessitate a change to cygport?

Not of itself.

The first paragraph describes the current state of affairs, since calm 
generates 'source:' lines, not 'Source:' lines.

This is preparatory work for possible noarching of source packages as 
discussed in [1].

[1] https://cygwin.com/ml/cygwin-apps/2017-04/msg00069.html

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On 01/05/2017 16:30, Ã…ke Rehnman wrote:
> Hello,
>
> I tested with my repo and it seem to fail if there is no setup.xz but
> only setup.ini. It does quickly realize there are no setup.xz and
> setup.xz.sig but then it takes forever to get past setup.bz2 etc and
> fails to download setup.ini
>
> These tests were done with -X flag.

Thanks very much for testing this.

It seems this could be an existing bug which could have been triggered 
the proxy case.

The attached incremental patch fixed this for me.

[-- Attachment #2: 0001-Close-rather-than-leak-InternetOpenUrl-handles-with-.patch --]
[-- Type: text/plain, Size: 572 bytes --]

From a30dab3f8c9081d922b7287801b882001077ddeb Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Mon, 1 May 2017 21:40:40 +0100
Subject: [PATCH setup] Close rather than leak InternetOpenUrl() handles with
 errors

---
 nio-ie5.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nio-ie5.cc b/nio-ie5.cc
index 3375c04..ba19ffa 100644
--- a/nio-ie5.cc
+++ b/nio-ie5.cc
@@ -122,6 +122,7 @@ try_again:
 	    }
 	  else if (type >= 300)
 	    {
+	      InternetCloseHandle (connection);
 	      connection = 0;
 	      return;
 	    }
-- 
2.12.2

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Åke Rehnman]

Hello all,

On 2017-05-01 22:45, Jon Turney wrote:
> It seems this could be an existing bug which could have been triggered 
> the proxy case.
>
> The attached incremental patch fixed this for me.

Seem to work fine for https and ftp now, I don't have the means to test 
ftps yet.

One thought though, why not let wininet take care of file:// URL's as 
well? Or actually don't try to parse the url string at all and just pass 
it down to NETIO_IE5 unfiltered? The advantage is setup would be able to 
handle what ever protocols wininet has. Also letting wininet taking care 
of file:// url's would let the user install from a local network 
resource (i.e file server). I'm been thinking of the case when someone 
wants to use a local directory repo would be slightly more complicated 
since relative paths does not work with file url's. One way to solve 
this particular case would be to check the first character for '.' and 
use that as an indicator to a local dir.

/Ake

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 02/05/2017 08:28, Ã…ke Rehnman wrote:
> Hello all,
>
> On 2017-05-01 22:45, Jon Turney wrote:
>> It seems this could be an existing bug which could have been triggered
>> the proxy case.
>>
>> The attached incremental patch fixed this for me.
>
> Seem to work fine for https and ftp now, I don't have the means to test
> ftps yet.

Thanks very much for testing.

> One thought though, why not let wininet take care of file:// URL's as
> well? Or actually don't try to parse the url string at all and just pass
> it down to NETIO_IE5 unfiltered? The advantage is setup would be able to

I'd be happy to look at a separate patch to do this.

> handle what ever protocols wininet has. Also letting wininet taking care
> of file:// url's would let the user install from a local network

I'm pretty sure I've done that in the past, so I think it already works. 
  The form of file: URL required might not be strictly correct, though, 
(I think file:////server/pathname/ ?)

I can't see how to get a file:// URL for a local directory to parse 
correctly, though.

> resource (i.e file server). I'm been thinking of the case when someone
> wants to use a local directory repo would be slightly more complicated
> since relative paths does not work with file url's. One way to solve
> this particular case would be to check the first character for '.' and
> use that as an indicator to a local dir.

Looking at the code, it seems that anything that is not recognized as a 
URL is just treated as a path, so this also may already be possible.

-- 
Jon Turney
Volunteer Cygwin/X X Server maintainer

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Åke Rehnman]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

Hi,

>> One thought though, why not let wininet take care of file:// URL's as
>> well? Or actually don't try to parse the url string at all and just pass
>> it down to NETIO_IE5 unfiltered? The advantage is setup would be able to
>
> I'd be happy to look at a separate patch to do this.
See proposed incremental patch. Have a look, give me your thoughts.

>
>> handle what ever protocols wininet has. Also letting wininet taking care
>> of file:// url's would let the user install from a local network
>
> I'm pretty sure I've done that in the past, so I think it already 
> works.  The form of file: URL required might not be strictly correct, 
> though, (I think file:////server/pathname/ ?)
Seem to work with \\server\share_name\path or //server/share_name/path 
now anyway

/Ake

[-- Attachment #2: 0001-Use-wininet-for-file-protocol.patch --]
[-- Type: text/plain, Size: 1725 bytes --]

From 4e8a95e59bf761bc4d0bcc4ba3ec042401073b54 Mon Sep 17 00:00:00 2001
From: Ake Rehnman <ake.rehnman at gmail.com>
Date: Tue, 2 May 2017 20:55:07 +0200
Subject: [PATCH 1/1] Use wininet for "file://" protocol. Use NetIO_File for
 unknown protocols.

---
 netio.cc    | 10 ++++++----
 nio-file.cc |  7 -------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/netio.cc b/netio.cc
index cf634c1..07ee217 100644
--- a/netio.cc
+++ b/netio.cc
@@ -126,7 +126,7 @@ NetIO::open (char const *url, bool cachable)
 {
   NetIO *rv = 0;
   enum
-  { http, https, ftp, ftps, file }
+  { http, https, ftp, ftps, file, unk }
   proto;
   if (strncmp (url, "http://", 7) == 0)
     proto = http;
@@ -136,10 +136,12 @@ NetIO::open (char const *url, bool cachable)
     proto = ftp;
   else if (strncmp (url, "ftps://", 7) == 0)
     proto = ftps;
-  else
+  else if (strncmp (url, "file://", 7) == 0)
     proto = file;
+  else
+    proto = unk;
 
-  if (proto == file)
+  if (proto == unk)
     rv = new NetIO_File (url);
   else if (net_method == IDC_NET_IE5)
     rv = new NetIO_IE5 (url, false, cachable);
@@ -157,7 +159,7 @@ NetIO::open (char const *url, bool cachable)
 	case ftp:
 	  rv = new NetIO_FTP (url);
 	  break;
-	case file:
+	case unk:
 	  rv = new NetIO_File (url);
 	  break;
 	default:
diff --git a/nio-file.cc b/nio-file.cc
index e69f1ff..c302616 100644
--- a/nio-file.cc
+++ b/nio-file.cc
@@ -35,13 +35,6 @@ NetIO (Purl)
     {
       file_size = get_file_size (std::string("file://") + path);
     }
-  else
-    {
-      const char *err = strerror (errno);
-      if (!err)
-	err = "(unknown error)";
-      note (NULL, IDS_ERR_OPEN_READ, path, err);
-    }
 }
 
 NetIO_File::~NetIO_File ()
-- 
2.12.2

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Brian Inglis]

On 2017-05-02 05:05, Jon Turney wrote:
> On 02/05/2017 08:28, Ã…ke Rehnman wrote:
>> On 2017-05-01 22:45, Jon Turney wrote:
> I'm pretty sure I've done that in the past, so I think it already 
> works. The form of file: URL required might not be strictly correct, 
> though, (I think file:////server/pathname/ ?)
> I can't see how to get a file:// URL for a local directory to parse 
> correctly, though.

URLs are like http://host/path - file://host/path; if no host is given, 
localhost is assumed in http:///path and file:///path

file:/// is the local root but some browsers accept just / e.g. lynx 
on Cygwin and Linux - useful to test this stuff, as GUI browsers look 
up URL history for completions. 
Most Windows browsers require the drive at the start of the path so 
should accept file:///d|/ (HTML originally allowed ":" only after the 
proto, and before the password and port in 
	proto://[[[user][:pswd]@]host[:port]]/path
so d| was required but that was relaxed in IE, and later other browsers 
followed), so likely file:///C:/, just C:, maybe C:/, rarely or never /.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 03/05/2017 08:22, Brian Inglis wrote:
> On 2017-05-02 05:05, Jon Turney wrote:
>> On 02/05/2017 08:28, Ã…ke Rehnman wrote:
>>> On 2017-05-01 22:45, Jon Turney wrote:
>> I'm pretty sure I've done that in the past, so I think it already
>> works. The form of file: URL required might not be strictly correct,
>> though, (I think file:////server/pathname/ ?)
>> I can't see how to get a file:// URL for a local directory to parse
>> correctly, though.
>
> URLs are like http://host/path - file://host/path; if no host is given,
> localhost is assumed in http:///path and file:///path
>
> file:/// is the local root but some browsers accept just / e.g. lynx
> on Cygwin and Linux - useful to test this stuff, as GUI browsers look
> up URL history for completions.
> Most Windows browsers require the drive at the start of the path so
> should accept file:///d|/ (HTML originally allowed ":" only after the
> proto, and before the password and port in
> 	proto://[[[user][:pswd]@]host[:port]]/path
> so d| was required but that was relaxed in IE, and later other browsers
> followed), so likely file:///C:/, just C:, maybe C:/, rarely or never /.

Yes.  Unfortunately, none of this works correctly in setup at the moment.

Incorrect URLs of the form 'file:////server/pathname/' work in setup, 
but my attempts to construct something setup would use as a file URL for 
a local directory were unsuccessful.

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 02/05/2017 20:29, Ã…ke Rehnman wrote:
>>> One thought though, why not let wininet take care of file:// URL's as
>>> well? Or actually don't try to parse the url string at all and just pass
>>> it down to NETIO_IE5 unfiltered? The advantage is setup would be able to
>>
>> I'd be happy to look at a separate patch to do this.
> See proposed incremental patch. Have a look, give me your thoughts.
>>
>>> handle what ever protocols wininet has. Also letting wininet taking care
>>> of file:// url's would let the user install from a local network
>>
>> I'm pretty sure I've done that in the past, so I think it already
>> works.  The form of file: URL required might not be strictly correct,
>> though, (I think file:////server/pathname/ ?)
> Seem to work with \\server\share_name\path or //server/share_name/path
> now anyway

Thanks for the patch. So there are a few separate things here:

* Pass unknown protocols to wininet

This seems a fine idea, but isn't what this patch does.

* Allow wininet to handle file:// URLs

I'm a little bit concerned that there may be current uses which rely on 
the incorrect parsing we do of file:// URLs to work.

Otoh, this should fix the file:// URL format we currently mishandle, so 
is probably worth doing.

* What to do with non-URL (i.e. pathname) addresses?

Perhaps WinInet can handle these, but assuming it does, is there a good 
reason to change from using NetIO_File()?

There are also some associated UI issues, in that we give no clue that a 
pathname is acceptable as a download site, and pathnames and file:// 
URLs are presented terribly in the download site list.

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 03/05/2017 18:08, Ã…ke Rehnman wrote:
> On 2017-05-03 18:37, Jon Turney wrote:
>> Thanks for the patch. So there are a few separate things here:
>>
>> * Pass unknown protocols to wininet
>>
>> This seems a fine idea, but isn't what this patch does.
> Yea, I know, I tested a few different solutions but it was difficult to
> make something clean and proper.

Ok.

>>
>> * Allow wininet to handle file:// URLs
>>
>> I'm a little bit concerned that there may be current uses which rely
>> on the incorrect parsing we do of file:// URLs to work.
>>
>> Otoh, this should fix the file:// URL format we currently mishandle,
>> so is probably worth doing.
> I don't know how that could have ever worked... (or does it?)

Not really, see the other branch of this thread.

>>
>> * What to do with non-URL (i.e. pathname) addresses?
>>
>> Perhaps WinInet can handle these, but assuming it does, is there a
>> good reason to change from using NetIO_File()?
> No. WinInet need proper file://host/path syntax. But the patch I made
> send those unknown protocols to NetIO_File(), right?

Yes.

I'll add a comment that "URLs we can't parse are assumed to be paths" 
and apply this, after I've  released 2.878.

Thanks for working on this.

>> There are also some associated UI issues, in that we give no clue that
>> a pathname is acceptable as a download site, and pathnames and file://
>> URLs are presented terribly in the download site list.
>>
> I saw that. Perhaps fixing that is another patch?

Sure.  I just mention that for completeness, not out of any expectation 
it's going to get fixed :)

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 04/05/2017 11:11, Jon Turney wrote:
> On 03/05/2017 18:08, Ã…ke Rehnman wrote:
>> On 2017-05-03 18:37, Jon Turney wrote:
>>>
>>> * Allow wininet to handle file:// URLs
>>>
>>> I'm a little bit concerned that there may be current uses which rely
>>> on the incorrect parsing we do of file:// URLs to work.
>>>
>>> Otoh, this should fix the file:// URL format we currently mishandle,
>>> so is probably worth doing.
>> I don't know how that could have ever worked... (or does it?)
>
> Not really, see the other branch of this thread.

So, I taking another look at this:

I am a bit confused about the behaviour I'm seeing, though.

I'm expecting "file:///c:/path" and "file://host/path" to work, but they 
don't seem to.

However, "file://c:/path" now works, which is good, and 
"file:////host/path" continues to work

The documentation for InternetOpenUrl() says "Only URLs beginning with 
ftp:, http:, or https: are supported" so I'm wondering if this is really 
something we should be expecting to work...

Also, is there a reason for the hunk in nio-file.cc removing the 
reporting of a problem opening the file?

Re: [PATCH setup 11/11] Use wininet for fetching URLs in direct (non-proxy) case (DO NOT APPLY)

[Jon Turney]

On 16/05/17 18:05, Ã…ke Rehnman wrote:
> On 2017-05-16 16:00, Jon Turney wrote:
>> I am a bit confused about the behaviour I'm seeing, though.
>>
>> I'm expecting "file:///c:/path" and "file://host/path" to work, but
>> they don't seem to.
> Same behavior here...

Since those are apparently the correct form of URLs, any ideas about how 
we might get them to work?

>> However, "file://c:/path" now works, which is good, and
>> "file:////host/path" continues to work
>>
>> The documentation for InternetOpenUrl() says "Only URLs beginning with
>> ftp:, http:, or https: are supported" so I'm wondering if this is
>> really something we should be expecting to work...
> Here is at least an enumeration of different schemes...
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa385149(v=vs.85).aspx

Hmm...  I'm not sure that's not just telling you what you can get back 
from InternetCrackUrl (which we should probably switch to using in 
NetIO::set_url() rather than doing things by hand)

>> Also, is there a reason for the hunk in nio-file.cc removing the
>> reporting of a problem opening the file?
>>
> It will complain for every setup file not found for instance if setup.xz
> is missing bug setup.ini is present. And besides, why should nio_file
> complain but not http and ftp?

Ah yes, I see the inconsistency now you point it out.  Makes sense.

I guess that should be changed to a log message.