Skip to content

Vendorize gems#38

Closed
georgebrock wants to merge 7 commits intogb-parserfrom
gb-vendorize-gems
Closed

Vendorize gems#38
georgebrock wants to merge 7 commits intogb-parserfrom
gb-vendorize-gems

Conversation

@georgebrock
Copy link
Copy Markdown
Collaborator

Adds support for vendorizing gem dependencies in the distribution tarball, installing them to $prefix/share/gitsh/gems, and requiring them from $prefix/bin/gitsh.

  • Vendorizing happens when ./configure is run, unless the vendorized gems are already present. Since the vendorized gems are always present in the tarball, this step is a no-op for end users. Doing it here gives us an explicit list of vendorized files we can use in vendor/Makefile.am to tell automake what to install.
  • The actual vendorizing process is handled by vendor/vendorize. This script will vendorize anything from Bundler's dist group, and build a setup.rb file which adds each gem's lib path to the $LOAD_PATH.
  • src/gitsh.in will include setup.rb making all of the gems available to the installed script.

@georgebrock
Copy link
Copy Markdown
Collaborator Author

There's a potential licensing problem here. Parslet depends on blankslate, which is MIT licensed but doesn't include the LICENSE file in the gem. There's an open PR to add a license, but the project hasn't been updated for a long time, and I'm pretty sure without the license file we'd be in breach.

@georgebrock
Copy link
Copy Markdown
Collaborator Author

Actually, I'm probably wrong: blankslate only contains one code file, which has permission to distribute it in a comment at the top of the file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I want to start a campaign to get your nickname internationally recognized as Sysexits.

George "Sysexits" Brocklehurst.

@georgebrock
Copy link
Copy Markdown
Collaborator Author

This vendorizing method still isn't quite right; if dependency versions change you need to manually delete the vendorized gems or the next distribution you build will contain the old versions.

This adds a vendor directory, whose Makefile knows how to vendorize all the
gems from the `:dist` bundler group, and write a `setup.rb` file to update
the `$LOAD_PATH` to include each gem.

The `vendor/gems` target is marked `PHONY` to avoid it being included in
`make all` and therefore happening on an end user's machine, however it is a
dependency of `make dist` so the vendorized gems will be included in the
distribution.

Automake doesn't support installing directories, so we use the
`install-data-hook` target to copy the gems directory to the right place
(prefix/share/gitsh/gems).
Installing via `nobase_dist_pkgdata_DATA` instead of using custom hooks.
This has a few advantages:

* `install-sh` takes care of the permissions for us.
* We don't need to manually uninstall.
* It's consistent with `lib/gitsh/Makefile.am`

The big discovery here was `nobase_` which preserves subdirectory
information, so the gems' files don't all end up directly in $pkgdatadir.
* The vendorize script needs to work relative to the current working
  directory in order to support VPATH builds.
* We need to remove `vendor/gems` as part of `make distclean`, since this is
  created by `make dist`
The improvements in 6f72fdd introduced a bug: If `vendor/gems` wasn't
present when `./configure` was run, then `$vendorfile` was empty, and so
`make dist` would vendorize and distribute the gems, but `make install`
wouldn't install them.

This change makes sure the vendorized gems are present in the configure
script, and will vendorize them if needed. The vendorized gems are included
in the distribution, so when installing from the tarball this is a no-op.
This is sort of reinventing make's functionality, but we need to vendorize
at configure time so that we have a full list of the vendorized files to
pass on to automake.
@georgebrock
Copy link
Copy Markdown
Collaborator Author

@mike-burns I think the latest commit fixes the problem with old dependencies being vendorized. Ready for another review.

configure.ac Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That subshell just took me down a fascinating and pedantic history lesson. Apparently there is no fully-compatible way of using head to get the first line from stdin!

@mike-burns
Copy link
Copy Markdown
Contributor

All the code is easy to follow and makes sense. Can't think of how to use make or auto* builtins to handle this.

Looks good to merge.

* Prefer $() to ``
* Quote expansions
@georgebrock georgebrock deleted the gb-vendorize-gems branch December 13, 2013 07:41
@georgebrock
Copy link
Copy Markdown
Collaborator Author

Merged!

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