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

added hie_bios_path_prefix attribute to haskell_repl #1531

Merged
merged 8 commits into from
Apr 14, 2021

Conversation

ylecornec
Copy link
Member

I added an attribute named hie_bios_path_prefix to the haskell_repl rule to address issue #1432.

This makes it possible for hie-bios to output paths relative to a different folder than the workspace root.

In order to use absolute paths, it is possible to use a placeholder in the prefix,
and then replace it with a call to pwd in the user defined .hie-bios script.

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

That looks good, thank you!

Could you add a test-case as well? We should test both cases: No prefix and some prefix.

Perhaps an sh_test that has a data dependency on a haskell_repl's hie-bios output would work.

@ylecornec
Copy link
Member Author

I added a test case which contains two hs_repl rules, one with the hie_bios_path_prefix option and one without which generate hie-bios files.

Then a sh_test rule check that the prefix appears in the first file, and that if we remove this prefix using sed, then the two files are equals.

Some notes :

  • Since the test uses a shell script, I added the dont_test_on_windows tag.
  • I also added a newline at the end of the hie-bios files since sed on mac complained about it.
  • I left some debug print which can be useful in case the test fails later, is this good practice ?

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

I left some debug print which can be useful in case the test fails later, is this good practice ?

I think it's better to not print this output in the success case and only print it in the failure case.


It looks like there's a conflict with master in haskell/repl.bzl. Could you rebase on master?

":hie-bios-file",
":hie-bios-file-no-prefix",
],
tags = ["dont_test_on_windows"],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tags = ["dont_test_on_windows"],

This shouldn't be necessary. rules_haskell requires msys2 on Windows anyway, so a bash is availabl. And Bazel can generate executables from .sh scripts on Windows if a bash is a available.

Copy link
Member

Choose a reason for hiding this comment

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

Just saw the Windows CI failure:

grep: tests/hie-bios/hie-bios@hie-bios: No such file or directory

I think the issue has to do with runfiles. On Unix Bazel will create a tree of symlinks in the tests working directory pointing to data dependencies. However, on Windows symlinks are not generally available, so instead Bazel generates a runfiles manifest file and code needs to use a runfiles library to query the manifest file for the location of data dependencies. The bash runfiles library is documented here. Example uses can be found in the repository. In tests you can use the TEST_WORKSPACE env-var to find the workspace name, e.g.

file1="$(rlocation "$TEST_WORKSPACE/$1")"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, thank you.

So I activated the windows test back on and it works with the bash runfiles library.
And now, debug information is only printed in case the test fails.

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you, looks good!

@aherrmann aherrmann added the merge-queue merge on green CI label Apr 14, 2021
@mergify mergify bot merged commit e2936bb into master Apr 14, 2021
@mergify mergify bot deleted the ylecornec/hie-bios-prefix branch April 14, 2021 07:39
@mergify mergify bot removed the merge-queue merge on green CI label Apr 14, 2021
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.

2 participants