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

Detecting duplicate dependencies in specification doesn't distinguish between runtime and development dependencies #1032

Merged
merged 2 commits into from Aug 26, 2015

Conversation

Projects
None yet
4 participants
@h-lame
Copy link
Contributor

h-lame commented Oct 3, 2014

03dbac9 (released in 2.2.0) introduced a feature where rubygems would error if a dependency was specified twice in a gem specification.

I think it might be too strict however as it doesn't seem to distinguish between runtime dependencies and development dependencies. It's not hard to come up with a scenario where a runtime dependency could be less strict than a development dependency and so you would want to specify the dependency twice.

For example: I work at a company that publishes a gem that provides some new matchers for rspec. The matchers work in any version of rspec, but our own specs for the gem are written against rspec 3. I'd expect to be able to specify that dependency graph as follows:

Gem::Specification.new do |gem|
  gem.add_runtime_dependency('rspec')
  gem.add_development_dependency('rspec', '>= 3.0')
end

However, issuing gem build errors out with:

WARNING:  open-ended dependency on rspec (>= 0) is not recommended
  if rspec is semantically versioned, use:
    add_runtime_dependency 'rspec', '~> 0'
WARNING:  See http://guides.rubygems.org/specification-reference/ for help
ERROR:  While executing gem ... (Gem::InvalidSpecificationException)
    duplicate dependency on rspec (>= 3.0, development), (>= 0) use:
    add_runtime_dependency 'rspec', '>= 3.0', '>= 0'

That it suggests the solution is to replace my add_runtime_dependency and add_development_dependency lines with a single add_runtime_dependency definitely seems wrong as that would end up making the gem only available to rspec 3 users when it works happily on rspec 1 or 2.

I think it would make sense to collect the seen dependencies depending on the dep.type and only error out if we specify a runtime dependency twice, or a development dependency twice, but allow us to specify a dependency with different constraints for runtime and development.

I'm happy to take a shot at working this up as a PR if you think it sounds like a good idea.

h-lame added a commit to unboxed/be_valid_asset that referenced this pull request Sep 30, 2014

Specify development dependencies in Gemfile.
Turns out that rubygems >= 2.2.0 doesn't allow duplicate dependencies in the gemspec.  I'm not sure if this is intentional behaviour when the "duplicates" are runtime vs. development dependencies (raised as rubygems/rubygems#1032), but even if it is wrong, we can't release the gem until a fix comes out.

We can just remove the development dependencies from the specification and define them in the project gemfile instead.  It's not as clear a signal, but it'll do for now.

@drbrain drbrain added this to the 2.5 milestone Oct 1, 2014

@drbrain

This comment has been minimized.

Copy link
Member

drbrain commented Oct 1, 2014

I agree with this bug report, but it should still report an error if the two requirements are not compatible.

I like your proposed solution and would appreciate a pull request. There should be adequate tests for you to work from, let me know here if you need extra assistance.

h-lame added some commits Oct 3, 2014

Allow for duplicates between runtime and development dependencies
We still detect duplicate runtime or duplicate development dependencies, but we allow for the same dependency to be specified as both a runtime and development dependency.
@h-lame

This comment has been minimized.

Copy link
Contributor

h-lame commented Oct 3, 2014

How's this? I appreciate the change is a bit noisy, but I ended up moving some things around so that raising an error would show all the error conditions rather than failing at the first hurdle. Let me know if you'd rather I a) didn't, b) did it in separate commits, c) something else.

You suggested that it still reports an error if the two requirements are not compatible so I wrote the following test:

def test_validate_dependencies_disallow_incompatible_runtime_vs_development_duplicates
  util_setup_validate

  Dir.chdir @tempdir do
    @a1.add_runtime_dependency 'b', '< 1.2.3'
    @a1.add_development_dependency 'b', '>= 1.2.3'

    use_ui @ui do
      e = assert_raises Gem::InvalidSpecificationException do
        @a1.validate
      end
    end

    assert_equal '', @ui.error, 'warning'
  end
end

However, when digging into the validate_dependencies method I realised that even if the specification was setup as follows:

@a1.add_runtime_dependency 'b', '< 1.2.3', '>= 1.2.3'

It still wouldn't raise. I even tried doing gem build with 2.4.1 on an gemspec that had a similar dependency, but it happily built the gem. It doesn't seem like rubygems currently cares about that sort of dependency incompatibility - at least, not at this stage. Is there somewhere else I should be looking?

@drbrain

This comment has been minimized.

Copy link
Member

drbrain commented Oct 3, 2014

I like your change. I'll merge this when I can next get some time.

Since RubyGems doesn't already complain about dependencies that are impossible to install I guess that should be a separate bug/pull request. Adding it to this one doesn't maintain topical consistency, IMO.

djberg96 added a commit that referenced this pull request Aug 26, 2015

Merge pull request #1032 from h-lame/detect-duplicate-dependencies-se…
…parately-for-runtime-vs-development

Detecting duplicate dependencies in specification doesn't distinguish between runtime and development dependencies

@djberg96 djberg96 merged commit 6169822 into rubygems:master Aug 26, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

djberg96 added a commit that referenced this pull request Aug 26, 2015

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