-
Notifications
You must be signed in to change notification settings - Fork 3
Add Shell Execution Extensions to Context #4
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
base: main
Are you sure you want to change the base?
Add Shell Execution Extensions to Context #4
Conversation
FrostingContextExtensions.cs
Outdated
{ | ||
private static readonly ProcessSettings _processSettings; | ||
|
||
static FrostingContextExtensions() => _processSettings = new ProcessSettings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do:
private static readonly ProcessSettings _processSettings = new();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 99399d0
FrostingContextExtensions.cs
Outdated
|
||
public static void SetShellWorkingDir(this FrostingContext context, string path) | ||
{ | ||
_processSettings.WorkingDirectory = path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to use the =>
thingy here if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 99399d0
FrostingContextExtensions.cs
Outdated
_processSettings.WorkingDirectory = path; | ||
} | ||
|
||
public static int ShellExecute(this FrostingContext context, string command, string environmentVariables = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass env variables in a separate function SetShellEnvVariables(parms string[] env)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 23d2fc1
From our previous discussion, the export
keyword is in fact required. Without it, the environment variables do not stay set if the command executes something that in turn executes something else. So the export
is required for the entire run of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I was not able to find any decent way of doing space character escapes in the command. The problem comes in situations where you want to set an environment variable such as
context.SetShellEnvironmentVariables("CFLAGS", $"-w -arch arm64 -I{dependencyDir}/include");
The space escapes would need to be set for the path in the -I
flag, however you would need to differentiate between what is a path and what isn't, such as the -w -arch arm64
part. Otherwise, you end up with a sanitized environment variable such as CFLAGS="-w\ -arch\ arm64\ -i/path/to\ some/directory/include;"
which fails to run.
Open to suggestions on this if still needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are supposed to set it to either:
CFLAGS=-w\ -arch\ arm64\ -i/path/to\ some/directory/include;
or
CFLAGS="-w -arch arm64 -i/path/to some/directory/include";
Adds extension methods for FrostingContext to help with executing shell commands per platform