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.

Sign up for GitHub

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

Jump to bottom

consul-ui: fix build #41239

Closed
wants to merge 2 commits into from
Closed

consul-ui: fix build #41239

wants to merge 2 commits into from

Conversation

jbboehr
Copy link
Contributor

@jbboehr jbboehr commented May 30, 2018 •
edited

Motivation for this change

consul-ui is currently failing to build in 18.03 and master (#41238). This is a PR against the release-18.03 branch because the fix currently does not work on the master branch.

I'm not sure if you allow PRs against the release branches, in which case feel free to close this.

Alternatively, you could merge into master anyway and backport to 18.03, since it's already broken in master.

I have not evaluated if #35602 fixes the issue.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@nh2
Copy link
Contributor

nh2 commented May 30, 2018

I have not evaluated if #35602 fixes the issue.

No, probably not, because I'm using that as well and with my latest rebase on release-17.09, I got the dreaded

Bundler will use `/tmp/nix-build-consul-ui-1.0.6.drv-0/bundler/home/root' as your home directory temporarily.
There was an error while trying to write to `/homeless-shelter/.gem/ruby/2.3.0`.
It is likely that you need to grant write permissions for that path.
make: *** [GNUmakefile:10: dist] Error 23
builder for '/nix/store/4h756nkxbn2fl4nzvqaflf2nnpx56hzx-consul-ui-1.0.6.drv' failed with exit code 2

After some significant confusion I figured out that the culprit is this in consul:

https://github.com/hashicorp/consul/blob/9a494b5fb9c86180a5702e29c485df1507a47198/ui/scripts/dist.sh#L18

bundle check >/dev/null 2>&1 || bundle install

First they hide all errors (like missing dependencies). Then call bundle install which fails because writing to HOME=/homeless-shelter is forbidden.

I really think we should patch that line out in nix or ask upstream to do it, because the >/dev/null 2>&1 hides the very helpful actual error message

The following gems are missing
* libv8 (3.16.14.15)

I also had some big trouble finding out how to fix that as it isn't really documented for consul-ui.

The trick (which you obviously know given the PR, so I'm writing this mainly for myself and others) is to do what it says on https://nixos.org/nixpkgs/manual/#sec-language-ruby, but in the ui subdir of the consul source and then to copy the files gemset.nix Gemfile Gemfile.lock created by that process into pkgs/servers/consul.

Not obvious at all. I think we should (in pkgs/servers/consul) create a README file that explains that, or a script that does that.

@nh2 nh2 mentioned this pull request May 30, 2018
Closed
@nh2
Copy link
Contributor

nh2 commented May 30, 2018

because the fix currently does not work on the master branch.

Why is that?

@nh2 nh2 mentioned this pull request May 30, 2018
Closed
8 tasks
nh2 added a commit to nh2/nixpkgs that referenced this pull request May 30, 2018
@nh2
consul: 0.9.3 -> 1.0.6; also upgrade python-consul...
83d90ff 
... to a version that can handle Consul >= 1.0.

See NixOS#35602 (comment)

Upgrading consul also requires upgrading the Ruby deps, see
NixOS#41239 (comment)

Also add instructions on how to do that, and a small patch
to improve the error message when it is forgotten.

I had to manually bump the Ruby `json` dependency from the generated
`1.8.2` to `1.8.5` to work around the
  ‘rb_cFixnum’ undeclared
problem as shown on
https://community.bitnami.com/t/gem-file-dependencies/50552.
nh2
nh2 reviewed May 30, 2018
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

Why is that?

Ah, I see, it's #41238, sorry.

pkgs/servers/consul/ui-lock.patch Outdated
@@ -0,0 +1,44 @@
diff --git a/ui/Gemfile.lock b/ui/Gemfile.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this patch instead of updating https://github.com/NixOS/nixpkgs/blob/deeeb5002c1ca27ca685e191f659e11f5ff01f4f/pkgs/servers/consul/gemset.nix with bundix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need both otherwise bundler tries to download and install a different version in dist.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nh2 I added a commit that patches out the bundler check|install|exec and it works without having to patch the lockfile

@jbboehr
Copy link
Contributor Author

jbboehr commented May 30, 2018

I've managed to fix it on the master branch, I'll open a new PR shortly.

@jbboehr jbboehr closed this May 30, 2018
@jbboehr jbboehr mentioned this pull request May 30, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nh2 nh2 nh2 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jbboehr @nh2 @GrahamcOfBorg