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

Support arguments to bios process #158

Closed
3noch opened this issue Mar 4, 2020 · 13 comments · Fixed by #188
Closed

Support arguments to bios process #158

3noch opened this issue Mar 4, 2020 · 13 comments · Fixed by #188
Assignees

Comments

@3noch
Copy link

3noch commented Mar 4, 2020

My bios script is just

#!/usr/bin/env bash
ob internal export-some-stuff > "$HIE_BIOS_OUTPUT"

It would be nicer if I could just inline this into hie.yaml directly with something like

cradle: { bios: { program: 'bash -c "ob internal export-some-stuff > \"$HIE_BIOS_OUTPUT\""' } }
@fendor
Copy link
Collaborator

fendor commented Mar 7, 2020

This ought to work in windows as well, right?
Possible implementation via shell

@jneira
Copy link
Member

jneira commented Mar 7, 2020

I think you could call cmd like bash

@fendor fendor self-assigned this Apr 7, 2020
@fendor
Copy link
Collaborator

fendor commented Apr 7, 2020

Proposed yaml:

cradle:
  bios:
    shell: 'bash -c "ob internal export-some-stuff > \"$HIE_BIOS_OUTPUT\""'

Maybe we can even ditch the OS dependant part and use just:

cradle:
  bios:
    shell: 'ob internal export-some-stuff > \"$HIE_BIOS_OUTPUT\"'

@mpickering
Copy link
Collaborator

Sounds sensible to me.

@jneira
Copy link
Member

jneira commented Apr 9, 2020

Not sure how can work that for windows and linux, using the same cradle config. One solution could be to have two files hie.yaml.linux and hie.yaml.windows and uncomment the desired one, but it is not ideal.
It happens too for the program case.

What about

cradle:
  bios:
    posix:
      shell: 'bash -c "ob internal export-some-stuff > \"$HIE_BIOS_OUTPUT\""'
    windows:
       shell: 'cmd /c "ob internal export-some-stuff > "%HIE_BIOS_OUTPUT%""

or

cradle:
  bios:
    posix:
      program: "./hadrian/hie-bios"
    windows:
      program: ".\\hadrian\\hie-bios.cmd"

Maybe we can even ditch the OS dependant part and use just:

We will not let the use of other shells like zsh in linux or cmd vs. powershell in windows, no?

@fendor
Copy link
Collaborator

fendor commented Apr 9, 2020

We will not let the use of other shells like zsh in linux or cmd vs. powershell in windows, no?

If the user choose to, with bash -c etc, then there is no reason against it.

cradle:
  bios:
    posix:
      program: "./hadrian/hie-bios"
    windows:
      program: ".\\hadrian\\hie-bios.cmd"

I kind of dislike the idea.
It is not a matter of posix vs windows. It is rather non-windows vs windows.
I think, the feature we actually think about here is some way of passing conditional flags, in this case conditional on the OS type.
However, implementing real conditionals is out of scope for this project in my opinion.
If you absolutely need to support multiple OS, I suggest that there is a disptach within the program, you specified in the hie.yaml.
Does that make sense? Or is that additional effort in the ConfigType worthwhile?


BTW, I forgot, the program receives a filepath to load. The command should be able to receive that as well. How should the shell API support that?

@jneira
Copy link
Member

jneira commented Apr 9, 2020

If you absolutely need to support multiple OS, I suggest that there is a dispatch within the program, you specified in the hie.yaml.

Mmm so hie-bios would choose one file or another for each os? looking for a file with the name and a executble extension?
Not sure if that solution will cover some corner cases but it could work

@fendor
Copy link
Collaborator

fendor commented Apr 10, 2020

Mmm so hie-bios would choose one file or another for each os? looking for a file with the name and a executble extension?

Not sure how that works, but arent we doing something comparable in HIE?
hie-cabal-install exists with and without extension, and the shell picks up the right file? Also, the script could be a Haskell or python script, which would avoid the problem entirely.

@jneira
Copy link
Member

jneira commented Apr 10, 2020

hie-cabal-install exists with and without extension, and the shell picks up the right file

the shell executes .bat and .cmd so if we use the shell to execute it may work (findExecutable does not i think)
but if the linux script is script.sh (or whateve extension) we would need a script.sh.bat 🤔

@fendor
Copy link
Collaborator

fendor commented Apr 14, 2020

Regarding

BTW, I forgot, the program receives a filepath to load. The command should be able to receive that as well. How should the shell API support that?

I have two ideas:
Either we interpolate the string, e.g.

cradle: 
  bios: 
    shell: 'bash -c \"produceFlags $HIE_ARG\" > \"$HIE_BIOS_OUTPUT\""'

and replace the string "$HIE_ARG" with the filepath name or we turn "$HIE_ARG" into an environment variable that you can use like in the example above or read in the script.

Pro for the first approach: Platform independent
Con: Brittle, probably hard to discover, maybe even hard to debug if there is a problem.

Pro for the second approach: Feels like it fits into the existing env variable approach
Con: Commands can be more easily platform dependent.

@hyperfekt
Copy link
Contributor

hyperfekt commented May 9, 2020

What's wrong with program: [ 'bash', '-c', 'produceFlags "$@" > "$HIE_BIOS_OUTPUT"', 'hie-bios-invoker' ]?
I'm not sure hie-bios needs to do anything but simply exec into the given program, just make sure it calls programs with their arguments, anything else can be solved by that program.
Cross platform functionality in hie-bios seems like a pretty orthogonal problem with a lot of added complexity that hie-bios is not necessarily in a better place to solve that than any other part of the pipeline.
Maybe a task like that is better suited to be solved by individual projects according to their requirements and by runtimes that are already intended to work cross-platform, prime among them GHC:
program: [ 'runhaskell', 'Flags.hs' ] (using stack (the interpreter) or cabal v2-run is also imaginable).

@fendor
Copy link
Collaborator

fendor commented May 9, 2020

program: [ 'bash', '-c', 'produceFlags "$@" > "$HIE_BIOS_OUTPUT"']

This looks good! Does the part $@ capture the filepath input parameter for the script?

Cross platform functionality in hie-bios seems pretty orthogonal with a lot of added complexity hie-bios is not necessarily in a better place to solve that than any other part of the pipeline.

I agree with that.

@hyperfekt
Copy link
Contributor

hyperfekt commented May 9, 2020

This looks good! Does the part $@ capture the filepath input parameter for the script?

Yes, it captures any argument to bash -c after the shell name (used for reporting errors), which needs to come after the command string.
Notably it should generally be in quotes so it doesn't merge them into a single one (not really relevant here but still good practice).

Alternatively, one can use $0 and omit the shell name.

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 a pull request may close this issue.

5 participants