Added environment variable resolution to the include directive in a config file #200

billh-apcon-com · Oct 6, 2016

Added environment variable resolution to the include directive in a config file

…onfig file

wilx · Oct 6, 2016

msvc14/Qt5DebugAppender.vcxproj

@@ -138,7 +138,11 @@
  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug_Unicode|Win32'">
    <Link>
      <AdditionalDependencies>Qt5Cored.lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalLibraryDirectories>C:\Qt\5.6.0\lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>


Do not add the changes to the Visual Studio project files to the pull request.

wilx · Oct 6, 2016

include/log4cplus/helpers/property.h

@@ -140,6 +140,7 @@ namespace log4cplus {

          // Methods
            void init(log4cplus::tistream& input);
+			bool substitueEnvVariables(tstring &fileName, tstring &modFileName);


Please remote all tabs from the source.

wilx · Oct 6, 2016

msvc14/log4cplus.sln

@@ -786,7 +768,6 @@ Global
 		{7EFABA06-71CD-498F-BF10-C41A7D2DCF3B} = {92DEA81D-81ED-4283-BBC7-41975647F69E}
 		{AE4CF05D-9770-4BF2-BB73-1DA37E2A1508} = {92DEA81D-81ED-4283-BBC7-41975647F69E}
 		{C0C6F651-99D8-404E-B2EC-507B261E820C} = {DAFB5F7D-6AF3-4D4F-B6D3-C1049A1259DA}
-		{18B64AA1-A2F7-46BE-8D48-7882AA471FA9} = {DAFB5F7D-6AF3-4D4F-B6D3-C1049A1259DA}


Do not add the changes to the Visual Studio project files to the pull request.

wilx · Oct 6, 2016

src/property.cxx

-        {
-            tstring included (buffer, 8) ;
-            trim_ws (included);
+		else if (buffer.compare(0, 7, LOG4CPLUS_TEXT("include")) == 0


Remove tabs.

wilx · Oct 6, 2016

src/property.cxx


            init (file);
        }
    }
 }

+bool
+Properties::substitueEnvVariables(tstring &fileName, tstring &modFileName)


I am not sure that we need a new function for this. There is already a function substVars() in configurator.cxx. It seems it should be used here as well. If so, you can probably move it from anonymous namespace into internal namespace and make it visible to the property.cxx and use it there.

billh-apcon-com · Oct 7, 2016

Sorry about the changes in the visual studio projects. I’m new to Git, and it’s a little different from other CMS packages.

I’ll look into your suggestion about substVars()

log4cplus/src/configurator.cxx

Line 102 in 078ee67

substVars (tstring & dest, const tstring & val,

.

For now, just reject the pull request.

Regards,
William Hayden
Apcon, Inc

From: Václav Haisman [mailto:notifications@github.com]
Sent: Thursday, October 06, 2016 3:55 PM
To: log4cplus/log4cplus log4cplus@noreply.github.com
Cc: Bill Hayden Bill.Hayden@apcon.com; Author author@noreply.github.com
Subject: Re: [log4cplus/log4cplus] Added environment variable resolution to the include directive in a config file (#200)

@wilx requested changes on this pull request.

In msvc14/Qt5DebugAppender.vcxprojhttps://github.com//pull/200#pullrequestreview-3201271:

@@ -138,7 +138,11 @@

 <Link>

   <AdditionalDependencies>Qt5Cored.lib;%(AdditionalDependencies)</AdditionalDependencies>

 <AdditionalLibraryDirectories>C:\Qt\5.6.0\lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>

Do not add the changes to the Visual Studio project files to the pull request.

In include/log4cplus/helpers/property.hhttps://github.com//pull/200#pullrequestreview-3201271:

@@ -140,6 +140,7 @@ namespace log4cplus {

       // Methods

         void init(log4cplus::tistream& input);

                 bool substitueEnvVariables(tstring &fileName, tstring &modFileName);

Please remote all tabs from the source.

In msvc14/log4cplus.slnhttps://github.com//pull/200#pullrequestreview-3201271:

@@ -786,7 +768,6 @@ Global

           {7EFABA06-71CD-498F-BF10-C41A7D2DCF3B} = {92DEA81D-81ED-4283-BBC7-41975647F69E}

           {AE4CF05D-9770-4BF2-BB73-1DA37E2A1508} = {92DEA81D-81ED-4283-BBC7-41975647F69E}

           {C0C6F651-99D8-404E-B2EC-507B261E820C} = {DAFB5F7D-6AF3-4D4F-B6D3-C1049A1259DA}

         {18B64AA1-A2F7-46BE-8D48-7882AA471FA9} = {DAFB5F7D-6AF3-4D4F-B6D3-C1049A1259DA}

Do not add the changes to the Visual Studio project files to the pull request.

In src/property.cxxhttps://github.com//pull/200#pullrequestreview-3201271:

@@ -256,27 +256,53 @@ Properties::init(tistream& input)

         trim_ws (value);

         setProperty(key, value);

     }

   else if (buffer.compare (0, 7, LOG4CPLUS_TEXT ("include")) == 0

```
       && buffer.size () >= 7 + 1 + 1
```
```
       && is_space (buffer[7]))
```
```
   {
```
```
       tstring included (buffer, 8) ;
```
```
       trim_ws (included);
```

         else if (buffer.compare(0, 7, LOG4CPLUS_TEXT("include")) == 0

Remove tabs.

In src/property.cxxhttps://github.com//pull/200#pullrequestreview-3201271:

         init (file);

     }

 }

}

+bool

+Properties::substitueEnvVariables(tstring &fileName, tstring &modFileName)

I am not sure that we need a new function for this. There is already a function substVars()

log4cplus/src/configurator.cxx

Line 102 in 078ee67

substVars (tstring & dest, const tstring & val,

in configurator.cxx. It seems it should be used here as well. If so, you can probably move it from anonymous namespace into internal namespace and make it visible to the property.cxx and use it there.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/200#pullrequestreview-3201271, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AVmKVf4EqvutvGeDG3KU1CmMe7_ml-T_ks5qxWArgaJpZM4KP7P4.

billh-apcon-com added 2 commits October 5, 2016 15:37

Added environment variable resolution to the include directive in a c…

18c9f67

…onfig file

Added environment variable resolution to the include directive in a c…

078ee67

…onfig file

wilx requested changes Oct 6, 2016

View reviewed changes

wilx self-assigned this Oct 6, 2016

wilx added the enhancement label Oct 6, 2016

murphi mentioned this pull request Sep 2, 2023

config: resolve environment variables in include #592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added environment variable resolution to the include directive in a config file #200

Added environment variable resolution to the include directive in a config file #200

billh-apcon-com commented Oct 6, 2016

wilx Oct 6, 2016

wilx Oct 6, 2016

wilx Oct 6, 2016

wilx Oct 6, 2016 •

edited

Loading

wilx Oct 6, 2016

billh-apcon-com commented Oct 7, 2016

Search code, repositories, users, issues, pull requests...

Added environment variable resolution to the include directive in a config file #200

Are you sure you want to change the base?

Added environment variable resolution to the include directive in a config file #200

Conversation

billh-apcon-com commented Oct 6, 2016

wilx Oct 6, 2016

Choose a reason for hiding this comment

wilx Oct 6, 2016

Choose a reason for hiding this comment

wilx Oct 6, 2016

Choose a reason for hiding this comment

wilx Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

wilx Oct 6, 2016

Choose a reason for hiding this comment

billh-apcon-com commented Oct 7, 2016

wilx Oct 6, 2016 •

edited

Loading