← Back to team overview

kicad-developers team mailing list archive

Re: PATCH: 3D resolver

 

I accidentally sent this patch only to Wayne when I meant to send it to
the list as well.

The attached patch fixes the issues with environment expansion by
using common.cpp ExpandEnvVarSubstitutions() to ensure
threadsafe invocation of wxGenEnv().

- Cirilo

On Fri, Apr 29, 2016 at 11:28 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
wrote:

> This is why ExpandEnvVarSubstitutions() in common.cpp is protected by a
> MUTEX for just this reason.  This was done specifically for loading the
> footprint libraries using multiple threads so it is thread safe.
>
> On 4/28/2016 5:05 PM, Cirilo Bernardo wrote:
> > Hi Wayne,
> >
> >  In fp_lib_table.cpp the FP_LIB_TABLE::ExpandEnvVarSubstitutions
> > invokes ExpandEnvVarSubstitutions in common.cpp. In common.cpp
> > there is this note:
> >
> > // wxGetenv( wchar_t* ) is not re-entrant on linux.
> >
> > That sort of thing makes me paranoid. Unfortunately POSIX getenv()
> > is not required to be reentrant and yet GNU don't seem to provide
> > a reentrant option or better still just make it reentrant. It will take
> me
> > a little more time to convert the wxFileName::Normalize() instances
> > to use ExpandEnvVarSubstitutions. Ideally wxWidgets should put a
> > mutex on calls to getenv(), but those changes will take a long time
> > to get to users.
> >
> > - Cirilo
> >
> >
> > On Thu, Apr 28, 2016 at 8:44 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx
> > <mailto:stambaughw@xxxxxxxxx>> wrote:
> >
> >     Cirilo,
> >
> >     We already have code that expands environment variables.  It's the
> >     static function:
> >
> >     const wxString FP_LIB_TABLE::ExpandSubstitutions( const wxString&
> >     aString )
> >
> >     I would prefer that you use it rather than write your own unless it
> >     doesn't suite your needs.  Looking at your patch, it should work
> >     just fine.
> >
> >     Thanks,
> >
> >     Wayne
> >
> >
> >     On 4/24/2016 2:06 AM, Cirilo Bernardo wrote:
> >     > The attached patch fixes a cut/paste syntactic error present
> >     > in all previous patches. A do{} block was changed to if{} but
> >     > the while() expression at the end was not removed.
> >     >
> >     > - Cirilo
> >     >
> >     >
> >     > On Sun, Apr 24, 2016 at 9:06 AM, Cirilo Bernardo
> >     > <cirilo.bernardo@xxxxxxxxx <mailto:cirilo.bernardo@xxxxxxxxx>
> >     <mailto:cirilo.bernardo@xxxxxxxxx
> >     <mailto:cirilo.bernardo@xxxxxxxxx>>> wrote:
> >     >
> >     >     The attached revised patch (against r6710) adds support for
> $(ENV_VAR)
> >     >     and fixes the bug reported by easyw: "invalid filename" on
> MSWin when
> >     >     manually editing a file name containing ${ENV_VAR}.
> >     >
> >     >     - Cirilo
> >     >
> >     >
> >     >     On Sat, Apr 23, 2016 at 5:29 PM, jp charras <
> jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>
> >     >     <mailto:jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>>>
> wrote:
> >     >
> >     >         Le 23/04/2016 09:16, Cirilo Bernardo a écrit :
> >     >         > Thanks Jean-Pierre,  I'll go through the 3D model code
> and make sure it can
> >     >         > use "${" and "$(".  I'm not sure about "%{", "%(" though
> since that wouldn't
> >     >         > be portable.  In principle there can also be multiple
> substitutions and a
> >     >         > substitution in the middle of a string such as
> >     >         > "/path/${VAR1}/${VAR2}/model.wrl"
> >     >         > but I think the current system should work fine; if
> people do something else
> >     >         > they can be responsible for the resulting behavior.
> >     >         >
> >     >         > - Cirilo
> >     >
> >     >         Supporting both "${" and "$(" is enough for me.
> >     >         If "${" is the starting point, "}" is the end of var name,
> and
> >     >         for "$(" the end of var name is ")"
> >     >
> >     >
> >     >         >
> >     >         > On Sat, Apr 23, 2016 at 4:28 PM, jp charras
> >     >         <jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>
> >     <mailto:jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>>>
> wrote:
> >     >         >
> >     >         >> Le 23/04/2016 01:29, Cirilo Bernardo a écrit :
> >     >         >>> Hi Wayne,
> >     >         >>>
> >     >         >>>  The attached patch was made against r6709.
> >     >         >>>
> >     >         >>> - Cirilo
> >     >         >>
> >     >         >> Cirilo, I just had a look at the patch, and noticed a
> >     (minor)
> >     >         issue:
> >     >         >> you are searching an ENV VAR by searching "${"
> >     >         >>
> >     >         >> This is perfectly true, but incomplete.
> >     >         >> wxWidgets accepts both "${" and "$(". (and on windows
> >     accept
> >     >         also "%{" and
> >     >         >> "%(" and more)
> >     >         >> The delimiters can be { and } or ( and )
> >     >         >> They are equivalent in wxWidgets, and allows using ) or
> >     } in
> >     >         paths
> >     >         >>
> >     >         >> (have a look at wxString wxExpandEnvVars(const
> >     wxString& str) in
> >     >         >> src/common/config.cpp, perhaps it
> >     >         >> could be used )
> >     >         >>
> >     >         >>
> >     >         >> This in important because we are using both in Kicad.
> >     >         >> for instance the fp lib wizard uses $(xxx) and in docs
> >     we use
> >     >         ${xxx}
> >     >         >> notation
> >     >         >>
> >     >         >> Thanks.
> >     >         >>
> >     >         >>>
> >     >         >>>
> >     >         >>> On Sat, Apr 23, 2016 at 12:52 AM, Wayne Stambaugh
> >     >         <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>
> >     <mailto:stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>>>
> >     >         >>> wrote:
> >     >         >>>
> >     >         >>>> Cirilo,
> >     >         >>>>
> >     >         >>>> This patch no longer applies cleanly.  Would you
> please fix
> >     >         it an repost
> >     >         >>>> it.
> >     >         >>>>
> >     >         >>>> Thanks,
> >     >         >>>>
> >     >         >>>> Wayne
> >     >         >>>>
> >     >         >>
> >     >         >>
> >     >         >> --
> >     >         >> Jean-Pierre CHARRAS
> >     >         >>
> >     >         >> _______________________________________________
> >     >         >> Mailing list: https://launchpad.net/~kicad-developers
> >     >         >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >     >         <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> >     >         >> Unsubscribe : https://launchpad.net/~kicad-developers
> >     >         >> More help   : https://help.launchpad.net/ListHelp
> >     >         >>
> >     >         >
> >     >
> >     >
> >     >         --
> >     >         Jean-Pierre CHARRAS
> >     >
> >     >
> >     >
> >     >
> >     >
> >     > _______________________________________________
> >     > Mailing list: https://launchpad.net/~kicad-developers
> >     > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >     > Unsubscribe : https://launchpad.net/~kicad-developers
> >     > More help   : https://help.launchpad.net/ListHelp
> >     >
> >
> >
> >     _______________________________________________
> >     Mailing list: https://launchpad.net/~kicad-developers
> >     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >     Unsubscribe : https://launchpad.net/~kicad-developers
> >     More help   : https://help.launchpad.net/ListHelp
> >
> >
>
>
=== modified file '3d-viewer/3d_cache/3d_cache.cpp'
--- 3d-viewer/3d_cache/3d_cache.cpp	2016-04-21 07:19:08 +0000
+++ 3d-viewer/3d_cache/3d_cache.cpp	2016-05-01 00:06:26 +0000
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2015 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
+ * Copyright (C) 2015-2016 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -32,8 +32,6 @@
 #include <wx/datetime.h>
 #include <wx/filename.h>
 #include <wx/log.h>
-#include <wx/thread.h>
-#include <wx/utils.h>
 #include <wx/stdpaths.h>
 
 #include <boost/uuid/sha1.hpp>
@@ -43,13 +41,13 @@
 
 #include "3d_cache.h"
 #include "3d_info.h"
+#include "common.h"
 #include "sg/scenegraph.h"
 #include "3d_filename_resolver.h"
 #include "3d_plugin_manager.h"
 #include "plugins/3dapi/ifsg_api.h"
 
 
-#define CACHE_CONFIG_NAME wxT( "cache.cfg" )
 #define MASK_3D_CACHE "3D_CACHE"
 
 static wxCriticalSection lock3D_cache;
@@ -225,19 +223,23 @@
     if( mi != m_CacheMap.end() )
     {
         wxFileName fname( full3Dpath );
-        wxDateTime fmdate = fname.GetModificationTime();
         bool reload = false;
 
-        if( fmdate != mi->second->modTime )
+        if( fname.FileExists() )
         {
-            unsigned char hashSum[20];
-            getSHA1( full3Dpath, hashSum );
-            mi->second->modTime = fmdate;
+            wxDateTime fmdate = fname.GetModificationTime();
 
-            if( !isSHA1Same( hashSum, mi->second->sha1sum ) )
+            if( fmdate != mi->second->modTime )
             {
-                mi->second->SetSHA1( hashSum );
-                reload = true;
+                unsigned char hashSum[20];
+                getSHA1( full3Dpath, hashSum );
+                mi->second->modTime = fmdate;
+
+                if( !isSHA1Same( hashSum, mi->second->sha1sum ) )
+                {
+                    mi->second->SetSHA1( hashSum );
+                    reload = true;
+                }
             }
         }
 
@@ -538,7 +540,13 @@
     if( !m_ConfigDir.empty() )
         return false;
 
-    wxFileName cfgdir( aConfigDir, wxT( "" ) );
+    wxFileName cfgdir;
+
+    if( aConfigDir.StartsWith( "${" ) || aConfigDir.StartsWith( "$(" ) )
+        cfgdir.Assign( ExpandEnvVarSubstitutions( aConfigDir ), "" );
+    else
+        cfgdir.Assign( aConfigDir, "" );
+
     cfgdir.Normalize();
 
     if( !cfgdir.DirExists() )
@@ -620,9 +628,9 @@
     cfgpath.AssignDir( wxStandardPaths::Get().GetUserConfigDir() );
 
 #if !defined( __WINDOWS__ ) && !defined( __WXMAC__ )
-    wxString envstr;
+    wxString envstr = ExpandEnvVarSubstitutions( "XDG_CONFIG_HOME" );
 
-    if( !wxGetEnv( wxT( "XDG_CONFIG_HOME" ), &envstr ) || envstr.IsEmpty() )
+    if( envstr.IsEmpty() )
     {
         // XDG_CONFIG_HOME is not set, so use the fallback
         cfgpath.AppendDir( wxT( ".config" ) );

=== modified file '3d-viewer/3d_cache/3d_cache_wrapper.cpp'
--- 3d-viewer/3d_cache/3d_cache_wrapper.cpp	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/3d_cache_wrapper.cpp	2016-05-01 00:07:21 +0000
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2015 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
+ * Copyright (C) 2015-2016 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -23,7 +23,6 @@
 
 
 #include <common.h>
-#include <wx/thread.h>
 #include <pgm_base.h>
 #include "3d_cache_wrapper.h"
 

=== modified file '3d-viewer/3d_cache/3d_filename_resolver.cpp'
--- 3d-viewer/3d_cache/3d_filename_resolver.cpp	2016-04-21 07:19:08 +0000
+++ 3d-viewer/3d_cache/3d_filename_resolver.cpp	2016-05-01 00:08:15 +0000
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2015 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
+ * Copyright (C) 2015-2016 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -21,19 +21,16 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
  */
 
-#include <iostream>
 #include <sstream>
-#include <cstdlib>
 #include <cstring>
 #include <fstream>
 #include <sstream>
 #include <wx/filename.h>
 #include <wx/log.h>
-#include <wx/thread.h>
-#include <wx/utils.h>
 #include <wx/msgdlg.h>
 #include <pgm_base.h>
 
+#include "common.h"
 #include "3d_filename_resolver.h"
 
 // configuration file version
@@ -64,7 +61,13 @@
     if( aConfigDir.empty() )
         return false;
 
-    wxFileName cfgdir( aConfigDir, wxT( "" ) );
+    wxFileName cfgdir;
+
+    if( aConfigDir.StartsWith( "${" ) || aConfigDir.StartsWith( "$(" ) )
+        cfgdir.Assign( ExpandEnvVarSubstitutions( aConfigDir ), "" );
+    else
+        cfgdir.Assign( aConfigDir, "" );
+
     cfgdir.Normalize();
 
     if( false == cfgdir.DirExists() )
@@ -82,7 +85,13 @@
     if( aProjDir.empty() )
         return false;
 
-    wxFileName projdir( aProjDir, wxT( "" ) );
+    wxFileName projdir;
+
+    if( aProjDir.StartsWith( "${" ) || aProjDir.StartsWith( "$(" ) )
+        projdir.Assign( ExpandEnvVarSubstitutions( aProjDir ), "" );
+    else
+        projdir.Assign( aProjDir, "" );
+
     projdir.Normalize();
 
     if( false == projdir.DirExists() )
@@ -96,10 +105,9 @@
     if( m_Paths.empty() )
     {
         S3D_ALIAS al;
-        al.m_alias = _( "(DEFAULT)" );
-        al.m_pathvar = _( "${PROJDIR}" );
+        al.m_alias = "${KIPRJMOD}";
+        al.m_pathvar = "${KIPRJMOD}";
         al.m_pathexp = m_curProjDir;
-        al.m_description = _( "Current project directory" );
         m_Paths.push_back( al );
         m_NameMap.clear();
 
@@ -177,6 +185,7 @@
     m_Paths.push_back( lpath );
     wxFileName fndummy;
     wxUniChar psep = fndummy.GetPathSeparator();
+    bool hasKISYS3DMOD = false;
 
     // iterate over the list of internally defined ENV VARs
     // and add existing paths to the resolver
@@ -202,21 +211,29 @@
                 continue;
             }
 
-            fndummy.Assign( mS->second.GetValue(), "" );
-
-            if( !fndummy.DirExists() )
-            {
-                ++mS;
-                continue;
-            }
-
+            // ensure system ENV VARs supercede internally defined vars
             wxString tmp( "${" );
             tmp.Append( mS->first );
             tmp.Append( "}" );
+            wxString pathVal = ExpandEnvVarSubstitutions( tmp );
+
+            if( pathVal.empty() )
+            {
+                pathVal = mS->second.GetValue();
+
+                if( pathVal.StartsWith( "${" ) || pathVal.StartsWith( "$(" ) )
+                    pathVal = ExpandEnvVarSubstitutions( pathVal );
+            }
+
+            fndummy.Assign( pathVal, "" );
+            fndummy.Normalize();
+
+            if( tmp == "${KISYS3DMOD}" )
+                hasKISYS3DMOD = true;
 
             lpath.m_alias =  tmp;
             lpath.m_pathvar = tmp;
-            lpath.m_pathexp = mS->second.GetValue();
+            lpath.m_pathexp = fndummy.GetFullPath();
 
             if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
                 lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
@@ -227,6 +244,26 @@
         }
     }
 
+    // special case: if KISYSMOD is not internally defined but is defined by
+    // the system, then create an entry here
+    wxString envar = ExpandEnvVarSubstitutions( "${KISYS3DMOD}" );
+
+    if( !hasKISYS3DMOD && !envar.empty() )
+    {
+        lpath.m_alias = "${KISYS3DMOD}";
+        lpath.m_pathvar = "${KISYS3DMOD}";
+        fndummy.Assign( envar, "" );
+        fndummy.Normalize();
+        lpath.m_pathexp = fndummy.GetFullPath();
+
+        if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
+            lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
+
+        if( !lpath.m_pathexp.empty() )
+            m_Paths.push_back( lpath );
+
+    }
+
     if( !m_ConfigDir.empty() )
         readPathList();
 
@@ -268,6 +305,7 @@
 wxString S3D_FILENAME_RESOLVER::ResolvePath( const wxString& aFileName )
 {
     wxCriticalSectionLocker lock( lock3D_resolver );
+
     if( aFileName.empty() )
         return wxEmptyString;
 
@@ -289,43 +327,49 @@
     tname.Replace( wxT( "/" ), wxT( "\\" ) );
     #endif
 
-    // this case covers full paths and paths relative to
-    // the current working directory (which is not necessarily
+    // Note: variable expansion must be performed using a threadsafe
+    // wrapper for the getenv() system call. If we allow the
+    // wxFileName::Normalize() routine to perform expansion then
+    // we will have a race condition since wxWidgets does not assure
+    // a threadsafe wrapper for getenv().
+    if( tname.StartsWith( wxT( "${" ) ) || tname.StartsWith( wxT( "$(" ) ) )
+        tname = ExpandEnvVarSubstitutions( tname );
+
+    wxFileName tmpFN( tname );
+
+    // in the case of absolute filenames we don't store a map item
+    if( !aFileName.StartsWith( "${" ) && !aFileName.StartsWith( "$(" )
+        && !aFileName.StartsWith( ":" ) && tmpFN.IsAbsolute() )
+    {
+        tmpFN.Normalize();
+
+        if( tmpFN.FileExists() )
+            return tmpFN.GetFullPath();
+
+        return wxEmptyString;
+    }
+
+    // this case covers full paths, leading expanded vars, and paths
+    // relative to the current working directory (which is not necessarily
     // the current project directory)
-    if( wxFileName::FileExists( tname ) )
+    if( tmpFN.FileExists() )
     {
-        wxFileName tmp( tname );
-
-        if( tmp.Normalize() )
-            tname = tmp.GetFullPath();
-
+        tmpFN.Normalize();
+        tname = tmpFN.GetFullPath();
         m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
 
+        // special case: if a path begins with ${ENV_VAR} but is not in the
+        // resolver's path list then add it
+        if( aFileName.StartsWith( "${" ) || aFileName.StartsWith( "$(" ) )
+            checkEnvVarPath( aFileName );
+
         return tname;
     }
 
-    // at this point aFileName:
-    // a. is a legacy ${} shortened name
-    // b. an aliased shortened name
-    // c. cannot be determined
-
-    if( aFileName.StartsWith( wxT( "${" ) ) )
+    // if a path begins with ${ENV_VAR}/$(ENV_VAR) and is not resolved then the
+    // file either does not exist or the ENV_VAR is not defined
+    if( aFileName.StartsWith( "${" ) || aFileName.StartsWith( "$(" ) )
     {
-        wxFileName tmp( aFileName );
-
-        if( tmp.Normalize() )
-        {
-            tname = tmp.GetFullPath();
-            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
-            return tname;
-        }
-        else if( resolveVirtualEnv( aFileName, tname ) )
-        {
-            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-            return tname;
-        }
-
         if( !( m_errflags & ERRFLG_ENVPATH ) )
         {
             m_errflags |= ERRFLG_ENVPATH;
@@ -338,6 +382,10 @@
         return wxEmptyString;
     }
 
+    // at this point aFileName is:
+    // a. an aliased shortened name or
+    // b. cannot be determined
+
     std::list< S3D_ALIAS >::const_iterator sPL = m_Paths.begin();
     std::list< S3D_ALIAS >::const_iterator ePL = m_Paths.end();
 
@@ -345,49 +393,52 @@
     // note: this is not necessarily the same as the current working
     // directory, which has already been checked. This case accounts
     // for partial paths which do not contain ${KIPRJMOD}.
-    if( !sPL->m_pathexp.empty() )
+    // This check is performed before checking the path relative to
+    // ${KISYS3DMOD} so that users can potentially override a model
+    // within ${KISYS3DMOD}
+    if( !sPL->m_pathexp.empty() && !tname.StartsWith( ":" ) )
     {
-        wxFileName fpath( wxFileName::DirName( sPL->m_pathexp ) );
-        wxString fullPath = fpath.GetPathWithSep() + tname;
+        tmpFN.Assign( sPL->m_pathexp, "" );
+        wxString fullPath = tmpFN.GetPathWithSep() + tname;
+
+        if( fullPath.StartsWith( "${" ) || fullPath.StartsWith( "$(" ) )
+            fullPath = ExpandEnvVarSubstitutions( fullPath );
 
         if( wxFileName::FileExists( fullPath ) )
         {
-            wxFileName tmp( fullPath );
-
-            if( tmp.Normalize() )
-                tname = tmp.GetFullPath();
-
+            tmpFN.Assign( fullPath );
+            tmpFN.Normalize();
+            tname = tmpFN.GetFullPath();
             m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
 
             return tname;
         }
+
     }
 
-    // ${ENV_VAR} paths have already been checked; skip all but
-    // ${KISYS3DMOD}, since legacy behavior was to check if paths
-    // were relative to ${KISYS3DMOD}
-    while( sPL != ePL && sPL->m_alias.StartsWith( "${" ) )
+    // check the partial path relative to ${KISYS3DMOD} (legacy behavior)
+    if( !tname.StartsWith( ":" ) )
     {
-        if( sPL->m_alias == "${KISYS3DMOD}" )
+        wxFileName fpath;
+        wxString fullPath( "${KISYS3DMOD}" );
+        fullPath.Append( fpath.GetPathSeparator() );
+        fullPath.Append( tname );
+        fullPath = ExpandEnvVarSubstitutions( fullPath );
+        fpath.Assign( fullPath );
+
+        if( fpath.Normalize() && fpath.FileExists() )
         {
-            wxFileName fpath( wxFileName::DirName( sPL->m_pathexp ) );
-            wxString fullPath = fpath.GetPathWithSep() + tname;
-
-            if( wxFileName::FileExists( fullPath ) )
-            {
-                wxFileName tmp( fullPath );
-
-                if( tmp.Normalize() )
-                    tname = tmp.GetFullPath();
-
-                m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
-                return tname;
-            }
+            tname = fpath.GetFullPath();
+            m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
+            return tname;
         }
 
+    }
+
+    // ${ENV_VAR} paths have already been checked; skip them
+    while( sPL != ePL && ( sPL->m_alias.StartsWith( "${" )
+        || sPL->m_alias.StartsWith( "$(" ) ) )
         ++sPL;
-    }
 
     // at this point the filename must contain an alias or else it is invalid
     wxString alias;         // the alias portion of the short filename
@@ -416,6 +467,9 @@
             wxFileName fpath( wxFileName::DirName( sPL->m_pathexp ) );
             wxString fullPath = fpath.GetPathWithSep() + relpath;
 
+            if( fullPath.StartsWith( "${") || fullPath.StartsWith( "$(" ) )
+                fullPath = ExpandEnvVarSubstitutions( fullPath );
+
             if( wxFileName::FileExists( fullPath ) )
             {
                 wxFileName tmp( fullPath );
@@ -424,7 +478,6 @@
                     tname = tmp.GetFullPath();
 
                 m_NameMap.insert( std::pair< wxString, wxString > ( aFileName, tname ) );
-
                 return tname;
             }
         }
@@ -462,7 +515,13 @@
         tpath.m_pathvar.erase( tpath.m_pathvar.length() - 1 );
     #endif
 
-    wxFileName path( tpath.m_pathvar, wxT( "" ) );
+    wxFileName path;
+
+    if( tpath.m_pathvar.StartsWith( "${" ) || tpath.m_pathvar.StartsWith( "$(" ) )
+        path.Assign( ExpandEnvVarSubstitutions( tpath.m_pathvar ), "" );
+    else
+        path.Assign( tpath.m_pathvar, "" );
+
     path.Normalize();
 
     if( !path.DirExists() )
@@ -645,7 +704,8 @@
     std::list< S3D_ALIAS >::const_iterator sPL = m_Paths.begin();
     std::list< S3D_ALIAS >::const_iterator ePL = m_Paths.end();
 
-    while( sPL != ePL && sPL->m_alias.StartsWith( "${" ) )
+    while( sPL != ePL && ( sPL->m_alias.StartsWith( "${" )
+        || sPL->m_alias.StartsWith( "$(" ) ) )
         ++sPL;
 
     wxFileName cfgpath( m_ConfigDir, S3D_RESOLVER_CONFIG );
@@ -721,47 +781,69 @@
 }
 
 
-bool S3D_FILENAME_RESOLVER::resolveVirtualEnv( const wxString& aFileName, wxString& aFullPath )
+void S3D_FILENAME_RESOLVER::checkEnvVarPath( const wxString& aPath )
 {
-    aFullPath.clear();
-
-    if( !aFileName.StartsWith( "${" ) )
-        return false;
-
-    size_t eDelim = aFileName.find( '}' );
-
-    if( eDelim == wxString::npos || eDelim + 2 >= aFileName.length() )
-        return false;
-
-    wxString tPath = aFileName.substr( 0, eDelim + 1 );
-
+    bool useParen = false;
+
+    if( aPath.StartsWith( "$(" ) )
+        useParen = true;
+    else if( !aPath.StartsWith( "${" ) )
+        return;
+
+    size_t pEnd;
+
+    if( useParen )
+        pEnd = aPath.find( ")" );
+    else
+        pEnd = aPath.find( "}" );
+
+    if( pEnd == wxString::npos )
+        return;
+
+    wxString envar = aPath.substr( 0, pEnd + 1 );
+
+    // check if the alias exists; if not then add it to the end of the
+    // env var section of the path list
     std::list< S3D_ALIAS >::const_iterator sPL = m_Paths.begin();
     std::list< S3D_ALIAS >::const_iterator ePL = m_Paths.end();
 
     while( sPL != ePL )
     {
+        if( sPL->m_alias == envar )
+            return;
+
         if( !sPL->m_alias.StartsWith( "${" ) )
-            return false;
-
-        if( sPL->m_alias == tPath )
-        {
-            tPath.Append( aFileName.substr( eDelim + 2 ) );
-            wxFileName tFile( tPath );
-            tFile.Normalize();
-
-            if( tFile.FileExists() )
-            {
-                aFullPath = tFile.GetFullPath();
-                return true;
-            }
-
-            return false;
-        }
+            break;
 
         ++sPL;
     }
 
-    return false;
+    S3D_ALIAS lpath;
+    lpath.m_alias = envar;
+    lpath.m_pathvar = lpath.m_alias;
+    wxFileName tmpFN;
+
+    if( lpath.m_alias.StartsWith( "${" ) || lpath.m_alias.StartsWith( "$(" ) )
+        tmpFN.Assign( ExpandEnvVarSubstitutions( lpath.m_alias ), "" );
+    else
+        tmpFN.Assign( lpath.m_alias, "" );
+
+    wxUniChar psep = tmpFN.GetPathSeparator();
+    tmpFN.Normalize();
+
+    if( !tmpFN.DirExists() )
+        return;
+
+    lpath.m_pathexp = tmpFN.GetFullPath();
+
+    if( !lpath.m_pathexp.empty() && psep == *lpath.m_pathexp.rbegin() )
+        lpath.m_pathexp.erase( --lpath.m_pathexp.end() );
+
+    if( lpath.m_pathexp.empty() )
+        return;
+
+    m_Paths.insert( sPL, lpath );
+    return;
 }
 
 
@@ -802,7 +884,7 @@
             fname.Replace( wxT( "\\" ), wxT( "/" ) );
             #endif
 
-            if( sL->m_alias.StartsWith( "${" ) )
+            if( sL->m_alias.StartsWith( "${" ) || sL->m_alias.StartsWith( "$(" ) )
             {
                 // old style ENV_VAR
                 tname = sL->m_alias;
@@ -1010,6 +1092,7 @@
 
         lpath = filename.substr( 0, pos0 );
 
+        // check the alias for restricted characters
         if( wxString::npos != lpath.find_first_of( wxT( "{}[]()%~<>\"='`;:.,&?/\\|$" ) ) )
             return false;
 
@@ -1020,6 +1103,19 @@
     else
     {
         lpath = aFileName;
+
+        // in the case of ${ENV_VAR}|$(ENV_VAR)/path, strip the
+        // environment string before testing
+        pos0 = wxString::npos;
+
+        if( aFileName.StartsWith( "${" ) )
+            pos0 = aFileName.find( '}' );
+        else if( aFileName.StartsWith( "$(" ) )
+            pos0 = aFileName.find( ')' );
+
+        if( pos0 != wxString::npos )
+            lpath = aFileName.substr( pos0 + 1 );
+
     }
 
     if( wxString::npos != lpath.find_first_of( wxFileName::GetForbiddenChars() ) )

=== modified file '3d-viewer/3d_cache/3d_filename_resolver.h'
--- 3d-viewer/3d_cache/3d_filename_resolver.h	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/3d_filename_resolver.h	2016-05-01 00:49:01 +0000
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2015 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
+ * Copyright (C) 2015-2016 Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -99,12 +99,12 @@
     bool writePathList( void );
 
     /**
-     * Function resolveVirtualEnv
-     * extracts the ${ENV_VAR} component of aFileName and puts a
-     * resolved path in aFullPath if the ${ENV_VAR} exists in the
-     * alias list and the referenced file exists.
+     * Function checkEnvVarPath
+     * checks the ${ENV_VAR} component of a path and adds
+     * it to the resolver's path list if it is not yet in
+     * the list
      */
-    bool resolveVirtualEnv( const wxString& aFileName, wxString& aFullPath );
+    void checkEnvVarPath( const wxString& aPath );
 
 public:
     S3D_FILENAME_RESOLVER();

=== modified file '3d-viewer/3d_cache/3d_plugin_manager.cpp'
--- 3d-viewer/3d_cache/3d_plugin_manager.cpp	2016-04-05 10:32:22 +0000
+++ 3d-viewer/3d_cache/3d_plugin_manager.cpp	2016-04-30 08:44:20 +0000
@@ -285,7 +285,7 @@
     // Per definition a loadable "xxx.bundle" is similar to an "xxx.app" app
     // bundle being a folder with some special content in it. We obviously don't
     // want to have that here for our loadable module, so just use ".so".
-    nameFilter.Append( wxT( ".so" ) );
+    nameFilter.Append( ".so" );
 #endif
 
     wxString lp = wd.GetNameWithSep();
@@ -316,7 +316,13 @@
     if( aPath.empty() || !wxFileName::FileExists( aPath ) )
         return;
 
-    wxFileName path( aPath );
+    wxFileName path;
+
+    if( aPath.StartsWith( "${" ) || aPath.StartsWith( "$(" ) )
+        path.Assign( ExpandEnvVarSubstitutions( aPath ) );
+    else
+        path.Assign( aPath );
+
     path.Normalize();
 
     // determine if the path is already in the list
@@ -355,7 +361,13 @@
         aPath.ToUTF8() );
     #endif
 
-    wxFileName path( wxFileName::DirName( aPath ) );
+    wxFileName path;
+
+    if( aPath.StartsWith( "${" ) || aPath.StartsWith( "$(" ) )
+        path.Assign( ExpandEnvVarSubstitutions( aPath ), "" );
+    else
+        path.Assign( aPath, "" );
+
     path.Normalize();
 
     if( !wxFileName::DirExists( path.GetFullPath() ) )

=== modified file '3d-viewer/3d_cache/CMakeLists.txt'
--- 3d-viewer/3d_cache/CMakeLists.txt	2015-12-08 07:31:57 +0000
+++ 3d-viewer/3d_cache/CMakeLists.txt	2016-05-01 00:48:04 +0000
@@ -1,3 +1,1 @@
-# unset CMAKE_CXX_FLAGS because it's contaminated with too many options
-set( CMAKE_CXX_FLAGS "" )
 add_subdirectory( sg )

=== modified file '3d-viewer/3d_cache/dialogs/dlg_3d_pathconfig.cpp'
--- 3d-viewer/3d_cache/dialogs/dlg_3d_pathconfig.cpp	2016-04-17 22:35:32 +0000
+++ 3d-viewer/3d_cache/dialogs/dlg_3d_pathconfig.cpp	2016-04-30 07:49:12 +0000
@@ -47,7 +47,8 @@
         size_t listsize = rpaths->size();
         size_t listidx = 0;
 
-        while( rI != rE && (*rI).m_alias.StartsWith( "${" ) )
+        while( rI != rE && ( (*rI).m_alias.StartsWith( "${" )
+            || (*rI).m_alias.StartsWith( "$(" ) ) )
         {
             ++listidx;
             ++rI;

=== modified file '3d-viewer/3d_class.cpp'
--- 3d-viewer/3d_class.cpp	2016-04-06 00:32:50 +0000
+++ 3d-viewer/3d_class.cpp	2016-05-01 00:50:10 +0000
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -29,6 +29,7 @@
 
 #include "3d_viewer.h"
 #include "3d_struct.h"
+#include "common.h"
 #include "modelparsers.h"
 #include <kiway.h>
 #include <3d_cache.h>
@@ -136,8 +137,9 @@
     // if the m_Shape3DName is not an absolute path the default path
     // given by the environment variable KISYS3DMOD will be used
 
-    if( m_Shape3DName.StartsWith( wxT("${") ) )
-        m_Shape3DFullFilename = wxExpandEnvVars( m_Shape3DName );
+    if( m_Shape3DName.StartsWith( wxT("${") ) ||
+        m_Shape3DName.StartsWith( wxT("$(") ) )
+        m_Shape3DFullFilename = ExpandEnvVarSubstitutions( m_Shape3DName );
     else
         m_Shape3DFullFilename = m_Shape3DName;
 


Follow ups