Skip to content

Add exports for syslog api. #5149

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

Merged
merged 2 commits into from
Oct 20, 2017
Merged

Add exports for syslog api. #5149

merged 2 commits into from
Oct 20, 2017

Conversation

dantraMSFT
Copy link
Contributor

This change enables calling the syslog apis to support issue #3766

#5144 is dependent on this change and also requires the libpsl-native nuget package to be republished.

extern "C" void Native_CloseLog()
{
closelog();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

#include <syslog.h>
#include <nativesyslog.h>

extern "C" void Native_SysLog(int32_t priority, const char* message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many cpp files in the directory has "//! " comments. If it is best practice please add doc comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@dantraMSFT
Copy link
Contributor Author

@iSazonov, @daxian-dbw, @TravisEz13 FYI: This is blocking the syslog PR and generating a new nuget package. Thanks.

@daxian-dbw
Copy link
Member

daxian-dbw commented Oct 18, 2017

I don't have additional comments except for what @iSazonov has pointed out.
One question though -- from packaging's perspective, will this new native code pull in any new dependencies? (I assume not, but want to make sure :))

@dantraMSFT
Copy link
Contributor Author

@daxian-dbw There are no additional dependencies.

@iSazonov
Copy link
Collaborator

@dantraMSFT I don't see new commit.

@daxian-dbw
Copy link
Member

@dantraMSFT Can you please address @iSazonov's comments, then the PR can be approved and merged.

@dantraMSFT
Copy link
Contributor Author

PR update. D'oh!

@SteveL-MSFT SteveL-MSFT requested a review from mirichmo October 19, 2017 21:39
@daxian-dbw daxian-dbw merged commit 053d180 into PowerShell:master Oct 20, 2017
@TravisEz13 TravisEz13 mentioned this pull request Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants