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

Download Stack if possible #1011

Merged
merged 1 commit into from
Jul 27, 2019
Merged

Download Stack if possible #1011

merged 1 commit into from
Jul 27, 2019

Conversation

mboes
Copy link
Member

@mboes mboes commented Jul 25, 2019

If we don't find a recent enough Stack on the PATH, then just download
one locally and use that. Since this is happening in a workspace rule,
we have no choice but to use prebuilt binaries. This behaviour brings
us closer to what rules_jvm_external does, which likewise downloads
a tool used to compute the dependency graph.

NixOS users will of course need to continue providing Stack in the
shell, because the Stack bindists don't work there.

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.

Looks good, that's a nice feature.

return

# If we can't find Stack, download it.
(os, arch) = _get_platform(repository_ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Should we detect NixOS and fail gracefully in that case? The bindist will probably fail with the usual and quite misleading "no such file or directory" error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should put this behind a flag and only enable it for bindists

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to reliably detect NixOS. Maybe we could improve the error message in this case with hints? Being on NixOS will be the common case for why the binary failed...

If we don't find a recent enough Stack on the PATH, then just download
one locally and use that. Since this is happening in a workspace rule,
we have no choice but to use prebuilt binaries. This behaviour brings
us closer to what rules_jvm_external does, which likewise downloads
a tool used to compute the dependency graph.

NixOS users will of course need to continue providing Stack in the
shell, because the Stack bindists don't work there, or using the new
`stack` attribute.
@mboes
Copy link
Member Author

mboes commented Jul 27, 2019

I improved the error message for NixOS users, but also made it possible for Nix users to supply Stack via nixpkgs_package, so that adding Stack to the shell is no longer necessary even there.

@mboes mboes added the merge-queue merge on green CI label Jul 27, 2019
@mergify mergify bot merged commit dd074be into master Jul 27, 2019
@mergify mergify bot deleted the prebuilt-stack branch July 27, 2019 13:51
@mergify mergify bot removed the merge-queue merge on green CI label Jul 27, 2019
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.

3 participants