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

Set environment variables #6

Merged
merged 14 commits into from
Oct 7, 2017
Merged

Set environment variables #6

merged 14 commits into from
Oct 7, 2017

Conversation

pipe01
Copy link
Contributor

@pipe01 pipe01 commented Oct 6, 2017

As promised, I added support for environment variables, although they only work in the full .NET framework AFAIK.

@Tyrrrz
Copy link
Owner

Tyrrrz commented Oct 7, 2017

Thanks for PR.
Can you split it into two PRs for each feature because I'd like to review/discuss them separately.

@pipe01 pipe01 changed the title Set environment variables and not throw on cancel Set environment variables Oct 7, 2017
@@ -15,4 +15,16 @@
<DocumentationFile>bin\$(Configuration)\$(TargetFramework)\$(AssemblyName).xml</DocumentationFile>
</PropertyGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for adding these packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't compile with .NET Standard and Core, but it doesn't work anyway so I'll try to remove them.

CliWrap/Cli.cs Outdated
@@ -41,9 +42,9 @@ public Cli(string filePath)
{
}

private Process CreateProcess(string arguments)
private Process CreateProcess(string arguments, Dictionary<string, string> env = null)
Copy link
Owner

Choose a reason for hiding this comment

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

At this point, just pass ExecutionInput instead of two parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was gonna do that, thought you might prefer it this way,

CliWrap/Cli.cs Outdated
{
foreach (var item in env)
{
#if NET45
Copy link
Owner

Choose a reason for hiding this comment

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

This works in .net standard 2.0 too so use #if NET45 && NETSTANDARD2_0 or however you write the net std moniker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have absolutely no idea how those .NET things work, so sorry about that.

Copy link
Owner

Choose a reason for hiding this comment

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

Don't worry about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When compiling .NET Standard it says that the type isn't found, I guess it doesn't work.

Copy link
Owner

Choose a reason for hiding this comment

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

It definitely works for me. You sure you have the net standard 2.0 sdk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I'll install it and try again.

CliWrap/Cli.cs Outdated
@@ -118,7 +131,7 @@ public ExecutionOutput Execute(ExecutionInput input, CancellationToken cancellat
var stdOut = stdOutBuffer.ToString();
var stdErr = stdErrBuffer.ToString();

return new ExecutionOutput(process.ExitCode, stdOut, stdErr);
return new ExecutionOutput(process.ExitCode, stdOut, stdErr, cancellationToken.IsCancellationRequested);
Copy link
Owner

Choose a reason for hiding this comment

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

Forgot to remove the cancellation token thing here

/// <summary>
/// Environment variables for the process
/// </summary>
public Dictionary<string, string> EnvironmentVariables { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

IDictionary<string, string> please

Copy link
Contributor Author

@pipe01 pipe01 left a comment

Choose a reason for hiding this comment

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

Done

@Tyrrrz
Copy link
Owner

Tyrrrz commented Oct 7, 2017

Okay thank you. I'll finish the PR before merging.

@Tyrrrz
Copy link
Owner

Tyrrrz commented Oct 7, 2017

Please don't commit to this branch past this point. :p

@pipe01
Copy link
Contributor Author

pipe01 commented Oct 7, 2017

I have installed the SDK and it still doesn't compile for me, it can't find ProcessStartInfo.EnvironmentVariables.

@Tyrrrz Tyrrrz merged commit d5f5d65 into Tyrrrz:master Oct 7, 2017
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.

None yet

2 participants