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.

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

Fix the build with a shallow clone of RuboCop #39

Merged
merged 4 commits into from Aug 3, 2015

Conversation

bquorning
Copy link
Collaborator

Running the specs depends on being able to load RuboCop's spec helpers. Since the RuboCop gem no longer (as of v0.30.0) contains the spec files, we have to vendorize RuboCop.

We can make a git submodule, or we can make a git clone, and I have only heard bad things about using submodules in git. And making a shallow clone is relatively fast.

Potential issues:

  • The vendorized RuboCop and the RuboCop gem dependency may be out of sync, in a number of ways:
    • Travis will always checkout the master branch of RuboCop.
    • A locally vendorized RuboCop becomes outdated without warning.

@bquorning bquorning mentioned this pull request Jun 18, 2015
@bquorning bquorning force-pushed the fix-build branch 2 times, most recently from 50006bd to 80bbd10 Compare June 19, 2015 07:39
Dir["#{rubocop_gem_path}/spec/support/**/*.rb"].each { |f| require f }
rubocop_path = File.join(File.dirname(__FILE__), '../vendor/rubocop')
unless File.directory?(rubocop_path)
fail "Can't run specs without a local RuboCop checkout. Look in the README."
Copy link
Collaborator

Choose a reason for hiding this comment

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

There doesn't seem to be any README in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I was hoping to get feedback on the code before writing README (and PR description).

On 22/06/2015, at 23.44, Andy Waite notifications@github.com wrote:

In spec/spec_helper.rb:

@@ -2,8 +2,11 @@

require 'rubocop'

-rubocop_gem_path = Gem::Specification.find_by_name('rubocop').gem_dir
-Dir["#{rubocop_gem_path}/spec/support/*/.rb"].each { |f| require f }
+rubocop_path = File.join(File.dirname(FILE), '../vendor/rubocop')
+unless File.directory?(rubocop_path)

  • fail "Can't run specs without a local RuboCop checkout. Look in the README."
    There doesn't seem to be any README in this PR?


Reply to this email directly or view it on GitHub.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The README has been changed to include information about RuboCop’s spec helpers.

@bquorning
Copy link
Collaborator Author

Added README info and PR description.

@bquorning
Copy link
Collaborator Author

Ping @andyw8

@geniou
Copy link
Collaborator

geniou commented Jun 29, 2015

Thanks for the pull request! As soon as I have the time I'm having a look. Sorry that I have to let you wait, but its not forgotten..

@bquorning
Copy link
Collaborator Author

No problem :-)

@backus
Copy link
Collaborator

backus commented Jul 26, 2015

ping @geniou. Might make sense to make @bquorning a maintainer or point to his fork or something

@bquorning
Copy link
Collaborator Author

Rebased on master.

@geniou @andyw8 Could one of you please find time to look at this PR? I would like to have some feedback on whether this approach makes sense, of if I should try looking for other solutions to the build problem.

@andyw8
Copy link
Collaborator

andyw8 commented Jul 26, 2015

My gut feeling is that having to clone RuboCop is not so good. But I'm not really familiar enough with RuboCop to suggest a better approach.

@bquorning
Copy link
Collaborator Author

The problem is that in order to run the specs for rubocop-rspec, the spec helpers from rubocop are needed. And they are not shipped with the gem, only with the source.

@andyw8
Copy link
Collaborator

andyw8 commented Jul 26, 2015

Perhaps @bbatsov can suggest a better approach?

@bquorning
Copy link
Collaborator Author

We could add the spec helpers back into the gem?

@geniou
Copy link
Collaborator

geniou commented Aug 2, 2015

Sorry for my silence.. I'm really sorry for that and thanks for the pull request!

I get the problem but I also think the need of having to clone of rubocop is not perfect.. I see some options:

  • include rubocop clone (current proposal)
  • provide the rubocop test cases somehow - maybe by extention in a separate gem we and rubocop is using.. Therefore we would need an opinion of @bbatsov
  • find a solution to have our own test classes and don't need to rely on external once..

I personally would like the second one.. bur maybe for now we should go for the first one and then try to replace it by a better (long term) solution.. opinions?

@geniou
Copy link
Collaborator

geniou commented Aug 2, 2015

Just for reference the commit on rubocop that caused that problem: rubocop/rubocop@5e8199e

We could also - as work around - use a version of rubocop that still delivers the required files..

@bquorning
Copy link
Collaborator Author

Depending on an old version of RuboCop is not a good idea, I think. Wouldn't that make this gem useless for people using a newer version of RuboCop?

Another (temporary) solution would be to simply copy the RuboCop spec helpers into this project.

@geniou
Copy link
Collaborator

geniou commented Aug 2, 2015

I don't know if there is a solution where you have development dependencies different from the one on production..

I think it's more than only one file so coping is not a good solution as well.

@geniou
Copy link
Collaborator

geniou commented Aug 3, 2015

I'm going to merge this even in the knowledge that its not a perfect solution to have this vendor folder. But for now it looks like the (good) way to get the test green for now. I'm also creating a new issue to find a long term solution.

@bquorning thanks for the PR and all you afford!

geniou added a commit that referenced this pull request Aug 3, 2015
Fix the build with a shallow clone of RuboCop
@geniou geniou merged commit 9fa365f into rubocop:master Aug 3, 2015
@bquorning
Copy link
Collaborator Author

❤️ Thanks.

@backus
Copy link
Collaborator

backus commented Aug 3, 2015

😃

@bquorning bquorning deleted the fix-build branch August 3, 2015 18:43
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.

None yet

4 participants