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

Build vagrant from source #30952

Merged
merged 2 commits into from Jan 10, 2018
Merged

Build vagrant from source #30952

merged 2 commits into from Jan 10, 2018

Conversation

aneeshusa
Copy link
Contributor

@aneeshusa aneeshusa commented Oct 30, 2017 •
edited

Motivation for this change

I got tired enough of all the hacks in the Vagrant derivation to take an excursion into Ruby-land packaging. The end result isn't perfect, but IMO it's a great deal better.
See individual commit messages for details.

Other highlights:

  • New url source type for Ruby gems
  • Probably a mass-rebuild of Ruby-dependent packages (not sure)
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.

cc maintainers @lovek323 @globin @jgeerds @kamilchm
cc @zimbatm and @cstrahan as Ruby maintainers
cc @samueldr: I tried to preserve the completions from #30482, but didn't know how to test this

zimbatm
zimbatm reviewed Oct 30, 2017
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

Hey, first of all this is awesome. I am glad to see the omnibus go away.

Would you mind submitting the ruby changes separately so that they can be reviewed more easily? Once this is merged the vagrant package would go in next.

pkgs/development/tools/vagrant/default.nix Outdated
];

postInstall = let
# Why is this 2.3.0 and not ${ruby.version}?
Copy link
Member

Choose a reason for hiding this comment

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

yes, why is this hard-coded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to ${ruby.version.libDir}, which appears to be what I want here.

@adisbladis adisbladis mentioned this pull request Dec 1, 2017
Merged
8 tasks
@aneeshusa aneeshusa mentioned this pull request Dec 23, 2017
Merged
8 tasks
@aneeshusa
Copy link
Contributor Author

Rebased for Vagrant 2.0.1, which has native Virtualbox 5.2 support.
cc @zimbatm I've opened #33014 to split out the first three commits here with general Ruby changes for you to review more easily.
I've kept the 4th commit here, which is also a Ruby infra change that adds the new url source type. I basically just changed enough bits in that commit to make the Vagrant derivation (in the last commit) work, so the two are fairly coupled and I think it will be easier to review with that context available, as well as easier to make any improvements you suggest.

@aneeshusa
Copy link
Contributor Author

Also cc @ryanartecona @zzamboni @nixy: I'd love if you could test this branch out on Darwin if you get a chance, as I can't test that myself!

@zzamboni
Copy link
Contributor

@aneeshusa thanks for this - unfortunately I have not been able to test due to NixOS/nix#1583, which causes my Mac to freeze whenever I try to compile anything heavy-duty. I will test when I'm able to upgrade to 10.13.2 (my Mac is centrally managed at work, so I cannot upgrade yet) and report back.

@ryanartecona
Copy link
Contributor

Just tested this locally, and it works great for me! I got the result /nix/store/z0snfc044b5ha673s5pkqic7nwpdqc51-vagrant. I've tested it with vagrant global-status and bringing an existing vagrant VM I had up and down.

The "You appear to be running Vagrant outside of the official installers" warning is pretty annoying, but agreed on not trying too hard to hack around it. We could get it to stop printing by setting VAGRANT_INSTALLER_ENV=1 in a wrapper script, but that seems to have other implications inside vagrant code which will make it look inside $VAGRANT_INSTALLER_EMBEDDED_DIR for some stuff, and I don't understand those in depth. If anything, I imagine we could maintain a small patch to the vagrant executable to just comment out the line that prints that warning, so Nix users don't see an unnecessary warning all the time.

@zzamboni
Copy link
Contributor

zzamboni commented Jan 3, 2018

@aneeshusa I upgraded my Mac to 10.3.2 this morning so I now had a chance to test this branch - it seems to work perfectly well. As @ryanartecona mentioned, I get a warning about not running Vagrant from an official installer. I have for now suppressed it by setting VAGRANT_INSTALLER_ENV=1 in my environment - so far no side effects are apparent, but I will keep an eye on it and report back if anything breaks.

Thanks for your work on this!

@aneeshusa
Copy link
Contributor Author

Thanks for testing @ryanartecona @zzamboni!
I've rebased this on the a freshly-rebased #33014, and added a patch to not show the unofficial installation warning. See the commit message for details on why that is better than setting the VAGRANT_INSTALLER_ENV variable.

@zimbatm
Copy link
Member

zimbatm commented Jan 4, 2018

thanks @aneeshusa. I merged the main PR, let's wait 1 day to see how this goes. Ping me if I forget this PR.

@aneeshusa
ruby-modules: Add new url source type
0750c5a 
This just defers to `fetchurl` for fetching.

Additionally, update the `nix-bundle-install.rb` script to handle gems
installed from a path, i.e. those with a `url` source.
Some parts of that script have been disabled in the `path` case
that likely shouldn't be, but cause errors and aren't necessary to get
`vagrant` to work.
@aneeshusa
vagrant: Build from source
8c3553b 
This is not quite as elegant as using `bundlerApp`,
which I could not get working.
However, this still uses most of the Ruby infrastructure,
including stock bundix, and should be fairly reasonable to maintain.

This means no more hacks to work around wrong embedded binaries,
and no need for an old version of Ruby.

Note that `vagrant share` is no longer included,
as that functionality is closed-source
and not present in the upstream source code.

The Vagrant maintainers publish official Vagrant installers,
which they prefer people use as most platforms don't
have great support for pinning known-good dependencies.
When run outside one of the offical installers,
Vagrant normally prints a warning to that effect.
However, Vagrant does run outside the installer environment
(nominally to support Vagrant development),
and this has the effect of functioning better by respecting
OS certs and shared libraries,
as opposed to trying to use bundled versions.
To keep these postive side effects without having to see the warning
on every Vagrant invocation, patch out the call to print the warning.

Note that I have reset the maintainers since the implementation is
totally redone; I'm happy to re-add any of the current maintainers.
@ryanartecona
Copy link
Contributor

Ping me if I forget this PR.

ping @zimbatm :)

@zimbatm zimbatm merged commit c5c8f17 into NixOS:master Jan 10, 2018
@zimbatm
Copy link
Member

zimbatm commented Jan 10, 2018

thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm zimbatm left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aneeshusa @zzamboni @ryanartecona @zimbatm @GrahamcOfBorg