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 nixpkgs_go_configure #107

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

saksmt
Copy link
Contributor

@saksmt saksmt commented Mar 7, 2020

It's a mystery how it wasn't added yet since rules_docker require working go sdk and it doesn't work on NixOS because it's being downloaded as a binary

Copy link
Member

@mboes mboes left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. This should be useful for rules_haskell, where we simply set go_register_toolchains(go_version = "host") inside a Nix shell. Generally looks good.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
nixpkgs/toolchains/go.bzl Outdated Show resolved Hide resolved
tests/BUILD.bazel Outdated Show resolved Hide resolved
@mboes mboes requested a review from aherrmann March 7, 2020 19:56
@saksmt saksmt requested a review from mboes March 7, 2020 21:31
@thufschmitt
Copy link
Contributor

Really cool 👍 As @mboes said, we currently have a workaround (using go_version = "host"), but this is much cleaner.

This breaks the tests however − which unfortunately isn't caught by the CI because it is broken 😕 − because they still use the very old bazel 1.1 which isn't compatible with the version of rules_go used. I tried updating to 2.0 but this breaks the python tests. Maybe we can find something in between so that we can merge this PR quickly and fix the python stuff later

@aherrmann aherrmann mentioned this pull request Mar 9, 2020
@aherrmann
Copy link
Member

I tried updating to 2.0 but this breaks the python tests. Maybe we can find something in between so that we can merge this PR quickly and fix the python stuff later

I took a look at that failure. Fortunately it was easy to work around the issue: #108

Copy link
Contributor

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Just a couple of comments that I forgot to submit earlier today

nixpkgs/toolchains/go.bzl Outdated Show resolved Hide resolved
nixpkgs/toolchains/go.bzl Outdated Show resolved Hide resolved
@saksmt saksmt force-pushed the feature/nixpkgs_go_toolchain branch from b0cc53a to 21ad8ae Compare March 10, 2020 12:47
@saksmt
Copy link
Contributor Author

saksmt commented Mar 10, 2020

Pulled latest master, fixed docs (see comment below) and improved nix expression.

Fix in docs:
Unfortunately go_register_toolchains uses some sort of reflection/introspection and collects all rules filtering out those that aren't go_sdk so we can't just tell users to call nixpkgs_go_toolchain (since we can't make it a rule and otherwise we can't find out if nix is present or not) and then go_register_toolchains and expect everything to work fine on both nix and non-nix environments, so hacks around rule generation with dynamic loading still needed (that one I mean) 🙁 . That means average usefulness of this code drastically drops, but still it's way better then to have go installed on local machine for non-go developer just to be able to build docker images

@saksmt saksmt requested a review from thufschmitt March 10, 2020 12:58
Copy link
Contributor

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

It's quite unfortunate that we can't just call the rule and let the toolchain resolution do its job, but I agree that having this rule is still better than using it from the environment.

(wrt the hack you mention, it can be simplified a bit: see tweag/rules_haskell@bb9ef6b. Maybe some of this could even be upstreamed in rules_nixpkgs)

@thufschmitt thufschmitt merged commit ec32535 into tweag:master Mar 11, 2020
@thufschmitt
Copy link
Contributor

Thanks!

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.

4 participants