Skip to content

Conversation

heaths
Copy link

@heaths heaths commented Jul 26, 2021

@heaths
Copy link
Author

heaths commented Jul 26, 2021

Still need to wire up some UI and hook this into the build, but this MSI should be complete. I have, however, been unable to test this. I need libcrypto.dll and hoping a PR build might build it so I can get a binary without having to clone OpenSSL and build it myself.

@heaths
Copy link
Author

heaths commented Jul 27, 2021

Seems $env:APPVEYOR_BUILD_FOLDER was not defined: https://ci.appveyor.com/project/PowerShell/openssh-portable/builds/40141492#L715

Seen this before? Looking through the PR I can't see what else might've affected it.

@floh96
Copy link

floh96 commented Nov 30, 2021

@heaths could you rebase and resolve the merge conflict please?
Because of the merge conflict the ci build did not start.

</Component>
<Component>
<File Name="sshd_config_default">
<PermissionEx Sddl="O:BAG:SYD:PAI(A;;FA;;;SY)(A;;FA;;;BA)" />
Copy link

Choose a reason for hiding this comment

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

We can have read permissions to the authorized users.

Copy link
Author

Choose a reason for hiding this comment

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

This was copied from the scripts at the time of writing. Have they changed?

# Use VS2019 or VS2017 build tools if installed.
if (-not (Test-Path $sdkPath))
{
$packageName = "windows-sdk-8.1"
Copy link

Choose a reason for hiding this comment

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

@tgauth, can you please cross check if windows SDK 8.1 is required?
With your recent telemetry changes, we are using windows SDK 10?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the telemetry changes do require windows SDK 10, for the tracelogging header files.
@heaths, is windows SDK 8.1 a requirement for this?

Also - the initial merge conflict was caused by changes I made to the build script for telemetry, so I can help resolve to make sure all changes get incorporated.

Copy link
Author

Choose a reason for hiding this comment

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

No. This was based on the script at the time but this PR has been open and unmerged for a long time. IMO, would've been better to merge it and add the UI later so any changes to other source would've been accounted for.

Copy link

@bagajjal bagajjal left a comment

Choose a reason for hiding this comment

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

Appreciate for taking time to provide this functionality.
I have minor comments. Most of the code looks good to me.

Thank you.

@bagajjal
Copy link

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.

5 participants