Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Changed 'nokogiri' back to runtime dependency with version >= 1.6.#43

Merged
vishrutshah merged 1 commit intoAzure:masterfrom
katmsft:master
Aug 22, 2017
Merged

Changed 'nokogiri' back to runtime dependency with version >= 1.6.#43
vishrutshah merged 1 commit intoAzure:masterfrom
katmsft:master

Conversation

@katmsft
Copy link
Copy Markdown
Member

@katmsft katmsft commented Aug 18, 2017

This can resolve the issue where bundler failed to recognize Nokogiri as a installed dependency. However, this will cause customers who is currently using ruby version 1.9.3~2.2.0 to not be able to install this gem, unless a compatible version of Nokogiri is installed before azure-asm-core.

@msftclas
Copy link
Copy Markdown

@katmsft,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah left a comment

Choose a reason for hiding this comment

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

Although change LGTM!

I tested with this branch in the CI and it only failed for Ruby version 2.2.0 only but the reason seems something different. Could you please have a look.

https://travis-ci.org/Azure/azure-sdk-for-ruby/builds/266116337?utm_source=github_status&utm_medium=notification

Thanks!

s.homepage = 'https://github.com/Azure/azure-ruby-asm-core'
s.license = 'Apache License, Version 2.0'
s.files = `git ls-files`.split("\n").reject { |f| f.start_with?("test/unit") }
s.extensions = 'ext/mkrf_conf.rb'
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.

Should we delete the ext/mkrf_conf.rb file as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are absolutely right. Now that we rely on our guidance in Readme.md for user to install correct version of Nokogiri via Rubygems, and resolved Bundler version issue, we can remove that. I have updated the PR for the purpose.

s.add_development_dependency('rake', '~> 10.0')
s.add_development_dependency('timecop', '~> 0.7')
s.add_development_dependency('bundler', '~> 1.11')
if RUBY_VERSION < "2.1.0"
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.

If I understand correctly this was working before in correctly installing the nokogiri version. Wasn't it? or Is it still being picked up from ext/mkrf_conf.rb file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No it is no longer picked up from the file. We now updated the Readme.md so that user can resolve the issue manually. This is a limitation caused by Nokogiri and we are not able to provide any solution or workaround for the issue as long as Nokogiri is one of our dependency. It is a common case for projects that depends on it.

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.

Sounds good!

…ed documentation to resolve dependency issue to Readme for users with Ruby version lower than 2.2.0.
@katmsft
Copy link
Copy Markdown
Member Author

katmsft commented Aug 22, 2017

Fail with Ruby version 2.2.0 is a known issue reported in #5341 in bundler’s repo. The issue was fixed in bundler 1.14.1, and won’t happen for 1.14.0 if ruby version is newer than 2.2.4.
If you change your script of gem install bundler -v 1.14.0 to gem install bundler -v 1.14.1 the problem should be fixed. I have updated the PR accordingly.

Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah left a comment

Choose a reason for hiding this comment

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

Thanks a lot @katmsft for the PR and making the fix.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants