Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Fix the path to the Gemfile during evaluation. #4244

Merged
merged 3 commits into from
Jan 31, 2016

Conversation

dtognazzini
Copy link
Contributor

This is a continuation of #3349 Following up per: #3349 (comment)

The issue here is that paths used with Bundler::Dsl#gemspec will only work when the Gemfile being evaluated is Bundler.default_gemfile.

Passing a Gemfile other than Bundler.default_gemfile to Bundler::Dsl.evaluate will break uses of path: options in the Gemfile.

These changes update Bundler::Dsl to remember the Gemfile passed into eval_gemfile and use it to resolve relative paths.

@segiddins
Copy link
Member

Could you add test coverage for whatever issue this is fixing?

Donnie Tognazzini added 3 commits January 27, 2016 14:48
@dtognazzini
Copy link
Contributor Author

That's a good idea. It's a bit unclear to me where a test case should go. Can you point me to the right place?

@segiddins
Copy link
Member

Test running bundle install from a different directory using the --gemfile option?

@dtognazzini
Copy link
Contributor Author

@segiddins

I'm not sure that would catch this. Bundler supports that already by using the "default gemfile" which will be whatever is passed as the --gemfile option.

The issue is when you use a different Gemfile than the default.

And that begs the question... Why am I doing that?

I'm pulling in bundler to use Bundler::Definition and Bundler::LockfileParser to read Gemfiles files and Gemfile.lock files. I'm doing this in a gem that uses bundler to manage its development dependencies. So... when I run tests, the globals used throughout bundler (Bundler.root, Bundler.default_gemfile, etc.) reference the local development Gemfile. I have to change this global state temporarily via something like:

def with_bundler_using_gemfile
  original_gemfile_env = ENV["BUNDLE_GEMFILE"]
  ENV["BUNDLE_GEMFILE"] = @gemfile_path

  original_root = Bundler.root
  Bundler.instance_variable_set(:@root, Pathname.new(@base_path))
  Dir.chdir(@base_path) do
    yield
  end
ensure
  Bundler.instance_variable_set(:@root, original_root)
  ENV["BUNDLE_GEMFILE"] = original_gemfile_env
end

Then I can use bundler's classes:

with_bundler_using_gemfile do
  all_gems = Bundler::LockfileParser.new(File.read(@lockfile_path))
...

This is working for me at the moment. I'm not sure if these classes are intended to be used outside of bundler. I'd prefer to code to a public API.

Does bundler have a public API that I can code to?

Lastly, I think these changes have merit even without my use case.

Dsl#gemspec's use of Bundler.default_gemfile makes the assumption that the gemfile parameter to Dsl#eval_gemfile is Bundler.default_gemfile.

Similarly, Source::Path use of Bundler.root makes the assumption that path sources are only created for the Gemfile in Bundler.default_gemfile.

@segiddins
Copy link
Member

These classes arent public API, though. Honestly, I'd prefer to add a with_gemfile private method for stuff like that

@dtognazzini
Copy link
Contributor Author

These classes arent public API

Fair enough. I think these changes are improvements as they remove assumptions regarding global state.

I interpreted @indirect's response in the original PR as a green light toward making these changes happen.

Where can I find the public API?

I'd guess that the design is limited to support a single Gemfile per process. If that's the case, I shouldn't be using bundler at all to parse Gemfiles. I'll probably write my own parser to avoid bumping up against bundler's internal design.

These observations make me curious as to whether anybody has thought about creating a gem for interacting with Gemfile/Gemfile.lock files. I'm not sure if bundler itself will ever support being used in this fashion; though it could.

@segiddins
Copy link
Member

The bundler public API at the moment is the CLI, Bundler.setup, Bundler.require, and bundler/inline.

Again, I'd be happy to introduce a private API that makes this easier, as I proposed in my last comment, but it needs to be thoroughly tested, and implemented in such a way that we can support it while changing the internals of Bundler -- and even then, we wouldn't make it 100% public.

@dtognazzini
Copy link
Contributor Author

I'd be happy to introduce a private API that makes this easier

Great! How can I help?

@segiddins
Copy link
Member

Honestly, I'd prefer to add a with_gemfile private method for stuff like that

Ideally, it would also be usable for bundler/inline as well

@dtognazzini
Copy link
Contributor Author

I'm not sure the with_gemfile private API would be enough for my case. I'd like to use other classes with with_gemfile. Would those classes also be in the private API and be supported?

@segiddins
Copy link
Member

@dtognazzini what specifically would you like to be public? Right now, we're not willing to commit to making bundler internals public because it would be a significant maintainance burden for us.

@dtognazzini
Copy link
Contributor Author

For what I'm doing to work I need access to a with_gemfile method,Bundler::LockfileParser, and Bundler::Definition.build. Would all of these supported through a private API?

@segiddins
Copy link
Member

Supported officially? No, not really. Loosely "supported" so that they're very unlikely to break but we won't make any promises? Sure.

@dtognazzini
Copy link
Contributor Author

@segiddins thanks for your time here. I see that Appraisals has their own version of Bundler's DSL:
https://github.com/thoughtbot/appraisal/blob/54c7c4b5df552ef7540d4612c17dbbcc1079f47c/lib/appraisal/bundler_dsl.rb

I'll probably do something similar and avoid using Bundler internals altogether.

I'm curious as to whether anybody has thought about creating a gem for interacting with Gemfile/Gemfile.lock files. A utility gem that provides Gemfile/Gemfile.lock files parsing would be useful.

@dtognazzini
Copy link
Contributor Author

@segiddins How do I find out what is supported in the "private API"? That is, what things are in the set of "loosely 'supported' so that they're very unlikely to break but we won't make any promises"?

#3463 discusses a Bundler Plugin System. I'd guess that plugins would access the private API. On Rubygems I found the following uses by other gems:

Are these part of the "private API"?

@indirect
Copy link
Member

@dtognazzini Bundler.with_clean_env is part of the public API, and is documented and will be completely stable. Bundler.rubygems and Bundler::CLI.start are not public and may change in the future (we don't have any plans to change them at present, but we also haven't documented them or agreed to keep them stable).

@dtognazzini
Copy link
Contributor Author

@indirect Right on, thanks. I've found Bundler.with_clean_env here:
http://bundler.io/v1.3/man/bundle-exec.1.html

It's unclear to me where to find what's included in the public API and the "loosely supported" API. Can you point me to the right place?

@indirect
Copy link
Member

@dtognazzini unfortunately, we don't (yet) have API docs. Mostly because the public API is so small and specialized. I can't think of anything beyond Bundler.setup, Bundler.require, and Bundler.with_clean_env. (And the requirable files 'bundler', 'bundler/setup', and 'bundler/inline').

@dtognazzini
Copy link
Contributor Author

Thanks!

@indirect
Copy link
Member

This seems like a good change (in the sense that it allows instances of Bundler::DSL to function correctly independent of the Bundler.default_gemfile global). Thanks!

@homu r+

@homu
Copy link
Contributor

homu commented Jan 31, 2016

📌 Commit 8f8c1c4 has been approved by indirect

@homu
Copy link
Contributor

homu commented Jan 31, 2016

⌛ Testing commit 8f8c1c4 with merge 2f87aaf...

homu added a commit that referenced this pull request Jan 31, 2016
Fix the path to the Gemfile during evaluation.

This is a continuation of #3349 Following up per: #3349 (comment)

The issue here is that paths used with `Bundler::Dsl#gemspec` will only work when the Gemfile being evaluated is `Bundler.default_gemfile`.

Passing a Gemfile other than `Bundler.default_gemfile` to `Bundler::Dsl.evaluate` will break uses of `path:` options in the Gemfile.

These changes update `Bundler::Dsl` to remember the Gemfile passed into `eval_gemfile` and use it to resolve relative paths.
@homu
Copy link
Contributor

homu commented Jan 31, 2016

☀️ Test successful - status

@homu homu merged commit 8f8c1c4 into rubygems:master Jan 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants