Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using NLog #2783

Merged
merged 6 commits into from
Jan 30, 2020
Merged

Using NLog #2783

merged 6 commits into from
Jan 30, 2020

Conversation

celeron533
Copy link
Contributor

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Compiled the code with Visual Studio

  • Require translation update

  • Require document update (readme.md, wikipage, etc)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New feature

Description of your pull request and other information

Replace the current logger to NLog with nlog.config.
Now just using legacy Verbose log switch. In future it could be more detailed log levels.
Main executable file size is about 2.66 MB.

@celeron533 celeron533 requested review from chenshaoju, a user and Stzx January 27, 2020 09:52
@celeron533 celeron533 self-assigned this Jan 27, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

In future it could be more detailed log levels.

We can also rewrite LogForm to have better looking.

@chenshaoju
Copy link
Collaborator

The quick test is passed.
But there is a NLog.config file with Shadowsocks.exe same folder, It's correct?

@celeron533
Copy link
Contributor Author

But there is a NLog.config file with Shadowsocks.exe same folder, It's correct?

Yes, will be extracted when it does not exist.
Then SS calls NLog.LogManager to reload this config file to activate.
Next time, when start SS, LogManager will load this file as its nature behavior.

@ghost
Copy link

ghost commented Jan 29, 2020

Consider move config file into temporary directory and/or always extract it.

@celeron533
Copy link
Contributor Author

Consider move config file into temporary directory

Form NLog official document:

For a stand-alone *.exe application, files are searched as follows:

1. standard application configuration file (usually applicationname.exe.config)
2. applicationname.exe.nlog in application’s directory
3. NLog.config in application’s directory (Name sensitive; using docker dotnet core)
4. NLog.dll.nlog in a directory where NLog.dll is located (only if NLog isn't installed in the GAC)

Could also explicitly load from specific file.
NLog.LogManager.Configuration = new NLog.Config.XmlLoggingConfiguration("nlog.config");

However, it is not encouraged "always extract" the NLog.config file because advanced user may added their own targets or rules with log level in the file. (Unless we have break change that require rewrite the NLog config.)

For putting things in temp folder... Could not find a strong reason to argue why or why not do this.

@ghost
Copy link

ghost commented Jan 29, 2020

Question: Should we allow advanced user edit config:
How if user broke the config file? How if user removed LogForm target by accident?

If we allow user edit config, then we can keep current behavior.

@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="utf-8" ?>
<!-- configuration may reset after shadowsocks upgrade -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@studentmain here is the NLog config file. Allow user to modify some of the part but not encouraged. Once the code is merged, corresponding Wiki page would be created.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if user broke the config, we can say "We warned you". It won't be problem now.

@celeron533 celeron533 merged commit 94f00f6 into shadowsocks:master Jan 30, 2020
@celeron533 celeron533 deleted the nlog branch January 14, 2021 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants