-
Notifications
You must be signed in to change notification settings - Fork 62
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
add shell and dependency-shell attributes to bios cradle type #188
Conversation
c5f90f3
to
47f17ef
Compare
Awesome! |
The project test I added is intended to work on Windows, too, but I have no way to actually run it. EDIT: Looks like Windows CI succeeded. |
Hi! first of all, great work, the pr looks good but not sure if the test case is correct: in windows shells you have to refer to en vars with
Simply changing to the correct dos syntax make it work (well the double quotes are a litle bit weird but whatever) , so the code is correct:
But i am a liitle bit worried by the fact that the test should have failed |
And it shows one of the shortcomings of the cradle: in general, you will need one version for each type of shell you want to support. |
This little trick should allow using both Windows-specific and POSIX commands in one command string. I have no idea why the test was passing though or even what exactly its conditions are, since I just blindly adapted it from the others. Maybe the project tests aren't really as thorough as thought? |
@hyperfekt i think those tests dont check the compiler args returned by the cradle, only that loading the cradle returns In that case i would not worry much about. |
, Just (String biosDepsProgram) <- Map.lookup "dependency-program" x | ||
= return $ Bios (T.unpack biosProgram) (Just (T.unpack biosDepsProgram)) | ||
, Just biosCallable <- exclusive (stringTypeFromMap Program "program") (stringTypeFromMap Command "shell") | ||
, Just biosDepsCallable <- exclusive (stringTypeFromMap Program "dependency-program") (stringTypeFromMap Command "dependency-shell") |
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.
Looks like here you could have program
and dependency-shell
which I suppose is acceptable?
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.
Yes, that was intentional. I didn't see a good reason to disallow it, and I added a test and documentation explicitly for this kind of mixing.
Thanks, very nice patch. Just a couple of comments. |
This allows passing additional arguments or redirecting a program's output to collect flags without needing to add a script to the project. To accomplish that, it utilizes the process package's support for executing shell commands by passing the CreateProcess data structure used by it to readProcessWithOutputFile instead of an executable name and arguments.
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.
LGTM
Thank you, nice patch!
This changes
readProcessWithOutputFile
to take acreateProcess
datatype instead of a program with arguments so the bios cradle type can pass in a shell command.I tried to avoid additional imports. Please feel free to make changes as you see fit, ask questions or give feedback.
Fixes #158.