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

Add tests for bios dependencies #233

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Conversation

pepeiborra
Copy link
Contributor

I ran into issues trying to define component dependencies in my Bios cradles, and wrote a pair of failing tests to demonstrate.
Please check that I'm not missing anything, and let me know if there is an obvious fix.

@fendor
Copy link
Collaborator

fendor commented Aug 12, 2020

I can reproduce the issue. The "problem" is this call: https://github.com/mpickering/hie-bios/blob/master/src/HIE/Bios/Cradle.hs#L354
It essentially assumes that the files are written to the file pointed to by "HIE_BIOS_OUTPUT". I assume I missed to document this properly with the documentation overhaul.
So, this fix your issue, this works for me:

cradle:
  bios:
    shell: |
      :; echo "-Wall" >> $HIE_BIOS_OUTPUT
      :; echo "A" >> $HIE_BIOS_OUTPUT
      :; echo "B" >> $HIE_BIOS_OUTPUT
      :; exit 0
      ECHO "-Wall" >> %HIE_BIOS_OUTPUT%
      ECHO "A" >> %HIE_BIOS_OUTPUT%
      ECHO "B" >> %HIE_BIOS_OUTPUT%
    dependency-shell: |
      :; echo "hie.yaml" >> $HIE_BIOS_OUTPUT
      :; exit 0
      ECHO "hie.yaml" >> %HIE_BIOS_OUTPUT%
> hie-bios debug A.hs
Root directory:        /home/munin/Documents/haskell/hie-bios/tests/projects/simple-bios-shell
Component directory:   /home/munin/Documents/haskell/hie-bios/tests/projects/simple-bios-shell
GHC options:           -Wall A B
GHC library directory: CradleSuccess "/nix/store/vhpfihshqj6sq5f9m2825gzz97nb3qkf-ghc-8.8.3/lib/ghc-8.8.3"
GHC version:           CradleSuccess "8.8.3"
Config Location:       /home/munin/Documents/haskell/hie-bios/tests/projects/simple-bios-shell/hie.yaml
Cradle:                Cradle {cradleRootDir = "/home/munin/Documents/haskell/hie-bios/tests/projects/simple-bios-shell", cradleOptsProg = CradleAction: Bios}
Dependencies:          hie.yaml

Do you think it is enough to document this or do you prefer writing things to stdout?

@pepeiborra
Copy link
Contributor Author

Fixing the README is enough for me. Thanks for looking into this!

@pepeiborra
Copy link
Contributor Author

Actually, there is another issue for me. The documentation says that dependency-program is called with no arguments, but that is inconsistent with program and doesn't really work for us. Is that by accident or by design?

@pepeiborra
Copy link
Contributor Author

how do I fix the Windows tests here?

@fendor
Copy link
Collaborator

fendor commented Aug 14, 2020

Afaict, the windows tests are just broken. Unfortunately, I dont know why they are broken, but you dont have to fix the tests right now, since HEAD is broken, too.
I will debug them later today and report back.

@fendor
Copy link
Collaborator

fendor commented Aug 20, 2020

So, after some digging, it seems like the polyglot bios shell script does not work on CI. Did you test it on windows?
I think that, because in the PR #236, in the latest commit, the test has the following fail message:

   simple-bios-shell:                             FAIL (0.30s)
      Finding Cradle for: C:\Users\runneradmin\AppData\Local\Temp\hie-bios-test-6313e53742483925\B.hs
      Loading Cradle: Just "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\hie-bios-test-6313e53742483925\\hie.yaml"
      Get runtime GHC library directory                              (0.07s)
      Get runtime GHC version                                        (0.05s)
      Initialise Flags                                               (0.16s)
      Initial module load
      comp opts: ComponentOptions {componentOptions = ["\"-Wall\" "], componentRoot = "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\hie-bios-test-6313e53742483925", componentDependencies = ["\"hie.yaml\" "]} (0.01s)
        target ‘"-Wall" ’ is not a module name or a source file

When the hie.yaml file looks like this:

cradle:
  bios:
    shell: |
      ECHO "-Wall" >> %HIE_BIOS_OUTPUT%
      ECHO "A" >> %HIE_BIOS_OUTPUT%
      ECHO "B" >> %HIE_BIOS_OUTPUT%
    dependency-shell: |
      ECHO "hie.yaml" >> %HIE_BIOS_OUTPUT%

otherwise, the test-output is like this:

   simple-bios-shell:                             OK (0.55s)
      Finding Cradle for: C:\Users\runneradmin\AppData\Local\Temp\hie-bios-test-bd77a1b14cf6e038\B.hs
      Loading Cradle: Just "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\hie-bios-test-bd77a1b14cf6e038\\hie.yaml"
      Get runtime GHC library directory                              (0.06s)
      Get runtime GHC version                                        (0.03s)
      Initialise Flags                                               (0.16s)
      Initial module load
      comp opts: ComponentOptions {componentOptions = [], componentRoot = "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\hie-bios-test-bd77a1b14cf6e038", componentDependencies = []} (0.24s)
      (1,2)                                                          (0.05s)
      (2,2)

Seems like the shell does not work for the string:

ECHO "-Wall"
ECHO "A"
ECHO "B"

@pepeiborra
Copy link
Contributor Author

Thanks for investigating!
I have no Windows box at hand, only tried to extend the test with dependencies following the same pattern.

Can the test be fixed at all? If not, what's the solution, disable it on windows?

@jneira
Copy link
Member

jneira commented Aug 21, 2020

Can the test be fixed at all? If not, what's the solution, disable it on windows?

Well, at very least we could run two different test, one per os. Please dont disable it 😸

@fendor
Copy link
Collaborator

fendor commented Aug 21, 2020

To me, it seems like the shell script feature just does not work for windows. Unfortunately, I dont know whether this can be fixed.

To be clear, the following does:

cmd = concat
  [ "ECHO \"-Wall\"\r\n"
  , "ECHO \"A\"\r\n"
  , "ECHO \"B\"\r\n"
  ]

> runProcessWithExitCode (shell cmd) ""
(ExitSuccess,"\"-Wall\"\n","")

Second and third line are swallowed. Maybe we have to use something different to shell?

@jneira
Copy link
Member

jneira commented Aug 21, 2020

Yeah, it seems it only executes the first line 🤯 . To pass multiline strings to cmd seems to be an heroic effort (i am not able to do it in a dos shell and so suggests monstrosities like https://stackoverflow.com/a/269819/49554).

Well, the workaround could be to use only one line:

Prelude System.Process Lib> m = "ECHO -Wall && ECHO A  && ECHO B"
Prelude System.Process Lib> readCreateProcessWithExitCode (shell m) ""
(ExitSuccess,"-Wall \nA  \nB\n","")

So the os aware script hack will not work as is cause it needs to put the linux specific things first 😄

@jneira
Copy link
Member

jneira commented Aug 21, 2020

Well, maybe this reversed hack could work:

echo -n # > nul && ECHO "-Wall" >> %HIE_BIOS_OUTPUT%  && ECHO "A" >> %HIE_BIOS_OUTPUT% && ECHO "B" >> %HIE_BIOS_OUTPUT%
:; echo "-Wall" >> $HIE_BIOS_OUTPUT
:; echo "A" >> $HIE_BIOS_OUTPUT
:; echo "B" >> $HIE_BIOS_OUTPUT

You could drop :; cause the windows shell is going to ignore those lines anyways

@pepeiborra pepeiborra force-pushed the bios-deps branch 2 times, most recently from d69771b to 07ac94a Compare August 25, 2020 09:15
Co-authored-by: Javier Neira  <atreyu.bbb@gmail.com>
@pepeiborra
Copy link
Contributor Author

Can this be merged now?

@fendor fendor merged commit 55d8e9b into haskell:master Aug 26, 2020
fendor pushed a commit that referenced this pull request Aug 26, 2020
* Add tests for bios dependencies

* Update docs and fix tests

* Attempt to fix the Windows tests

* Eliminate whitespace in ECHO

Co-authored-by: Javier Neira  <atreyu.bbb@gmail.com>
@fendor
Copy link
Collaborator

fendor commented Aug 26, 2020

Thank you for your patience, merged it now

@pepeiborra
Copy link
Contributor Author

No worries, thank you and @jneira for your help!

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.

None yet

3 participants