Fix travis ci build #232

Merged
merged 7 commits into from Mar 6, 2013

Conversation

Projects
None yet
5 participants
Contributor

tmcgilchrist commented Mar 6, 2013

I've taken the liberty of messing with the gemfiles so that the travis-ci build passes.
I needed to move the gemfiles into the top level directory, for whatever reason travis-ci didn't like where they were.

I also added a ruby 1.9.3 target in the travis configuration.

rmm5t merged commit e30318e into thoughtbot:master Mar 6, 2013

1 check passed

default The Travis build passed
Details
Collaborator

rmm5t commented Mar 6, 2013

🍻 Thanks for this!

Contributor

gabebw commented Mar 6, 2013

@tmcgilchrist, I appreciate your work - but this probably should not have been merged, since the gemfiles are all auto-generated by appraisal, specifically the Appraisals file. I think a more general solution would be like the one we just added to shoulda-matchers: thoughtbot/shoulda-matchers#247 .

As you can see in the Rakefile, running rake will regenerate the gemfiles, which I believe will essentially revert the work done in this PR - it at least recreates the gemfile directory.

I'd love a bit more detail about why Travis didn't like gemfiles in in the gemfiles/ directory? We've been using Appraisal, and its gemfiles in gemfiles/ directory, on a bunch of projects (like Clearance) and I'd like to get any possible problems with Appraisal fixed.

Collaborator

rmm5t commented Mar 6, 2013

@gabebw My bad here. I was tired of seeing those build failures too, and I jumped the gun. Sorry about that. I will revert.

@rmm5t rmm5t added a commit that referenced this pull request Mar 6, 2013

@rmm5t rmm5t Revert "Merge pull request #232 from tmcgilchrist/fix_travis_ci"
This reverts commit e30318e, reversing
changes made to e2d8cae.
2c09e79
Collaborator

rmm5t commented Mar 6, 2013

Okay. Merge reverted.

@gabebw I also noticed that all the gemfile.lock files are checked in. This seems like a mistake, especially since they reference directory paths in a developer's environment. I'd like to .gitignore them from the project. Any objections?

Collaborator

rmm5t commented Mar 6, 2013

@gabebw Actually, one step further, because all the gemfiles are generated by appraisal, shouldn't the entire gemfiles directory be gitignored? I vote yes.

Contributor

gabebw commented Mar 6, 2013

@rmm5t Actually, all Gemfile.lock files and the gemfiles/ directory should be checked in, per the Appraisal instructions. The paths (like /Users/gabe/thoughtbot/open-source/appraisal) often change, but I've never seen Bundler do anything with that path anyway. Appraisal takes care of it for us.

Collaborator

rmm5t commented Mar 6, 2013

@gabebw Interesting. Bundler itself (through Yehuda) says that the Gemfile.lock should not be checked in for gems. If you need to lock down to a particular version for the tests, include more specific versions in the gemspec and/or the Appraisals file -- more appropriate for the Appraisals file in this case.

Also, since all the files in gemfiles/ are regenerated and overwritten every time the full shoulda test suite is run, doesn't that just emphasize why these files have no place in the repository and should be ignored? I.e. When the shoulda test suite is run, all the lock files are updated, because the default rake task calls appraisal:cleanup then appraisal:install.

I've botched then sidetracked this PR long enough, but seeing auto-generated files thrash after each developer runs a full test-suite just seems like a broken setup to me.

Contributor

gabebw commented Mar 6, 2013

Yes, we've had the discussion about that blog post a few times :)

@jferris (creator of Appraisal) might have a better answer here, but I believe the tradeoff of "this line in some files might change, just ignore it" is worth having reproducible regression testing.

Owner

jferris commented Mar 6, 2013

I've discussed the issue of lockfiles with Yehuda in person. He agreed that, if you have a process for reproducible scenarios across versions, it makes sense to check in the lockfiles.

The changing pathnames is an annoying issue that I haven't been able to fix yet. It doesn't break anything, though, so it keeps slipping down my priority list.

I wrote about the philosophy behind Appraisal on our blog: http://robots.thoughtbot.com/post/12068553267/appraisal-find-out-what-your-gems-are-worth

Collaborator

rmm5t commented Mar 6, 2013

@jferris I now remember reading your post back when you wrote it. Appraisal is a great way to maintain reproducible regression tests as they relate to dependencies; committing a lock file, not so much -- especially in the scenario described here where the lock file potentially changes on every full test suite run. Committing a file that can be generated from Appraisal, also not much of a win, IMO.

Also, in this scenario, it's not just about changing pathnames. The entirety of the lock files change on the full test-suite (assuming gem dependencies have released new versions from the time another developer last ran the suite).

To touch on why I disagree with the underlying philosophy of checking in lock files (for gems), consider this. At the end of the day, the most important combination of dependencies is the combination that represents the newest versions that adhere to the gemspec. This is because any new installs will probably be using this combination of dependencies in practice. If everyone is a good actor, SemVer conventions provide us great value here. However, by checking in the Gemfile.lock (or the locks generated by appraisal), you lose that most important edge case.

To touch on the case of being able to have reproducible regressions, in reality, the only way to get full coverage is to run the tests against every permutation allowed by the gemspec. This obviously could yield a ton of tests, and several useless combinations might fail and thus need to be ignored. This probably isn't all that practical at the end of the day either, because again, the most important combination of dependencies are the latest versions that adhere to the gemspec and the latest versions that adhere to each Appraisal block (because these represent common broad installation scenarios).

@tmcgilchrist Look what you started! 😈

Contributor

tmcgilchrist commented Mar 6, 2013

Oh dear what have I done 😄

I just looked at thoughtbot/clearance and the setup there doesn't seem much different to what's in this repo. I'll have another go at making it work with the current gemfiles setup.

Contributor

sferik commented Mar 16, 2013

👍 to @rmm5t’s suggestion to ignore the gemfiles directory and run tests with the current/latest versions that bundler resolves at testtime. This ought to help identify bugs and incompatibilities in new versions of dependencies faster than locking to known-working-but-possibly-outdated dependency versions.

See also: thoughtbot/shoulda-matchers#254, in which I ask why the shoulda-matchers dependency is specified with three digits of precision instead of two (this applies to the shoulda-context dependency as well). As long as those gems follow Semantic Versioning (no breaking changes in minor releases), you won't have to ship a new version of shoulda after a minor dependency release. If these two dependencies are shipping backward-incompatible changes in minor versions, they should stop doing that. 😉

I believe these two changes would go a long way toward keeping shoulda up-to-date and should lower the maintenance burden. This is basically why SemVer exists.

Collaborator

rmm5t commented Mar 17, 2013

Back to the underlying reason this PR was opened, there is now a more permanent fix in #235 that should help turn the builds green.

@tmcgilchrist Even though your original PR had to be reverted, thank you for starting this discussion. I'm not sure anything would have been started had you not initiated this. 🍻

Contributor

tmcgilchrist commented Mar 17, 2013

@rmm5t thanks, sometimes complaining and being picky does get results. 😁

Owner

jferris commented Mar 21, 2013

@sferik I think ignoring the gemfiles and version specifications are separate issues. I think the bourne dependency is better with a loose specification, although mocha (a dependency of bourne) doesn't follow SemVer, so it's not clear cut in this case.

@rmm5t re gemfiles:

Committing a file that can be generated from Appraisal, also not much of a win, IMO.

The lockfiles can't be generated from Appraisal; the lockfile contains a specific, reproducible scenario, whereas the Appraisal file (and the gemfiles it generates) contain less specific scenarios. Ignoring the lockfiles means that you'll end up with a different configuration on each checkout.

The most important combination of dependencies is the combination that represents the newest versions that adhere to the gemspec

I don't think that's self-evident. Most applications don't ignore lockfiles, and unless they constantly run bundle update, they'll almost never be using the latest versions of anything. Because of compatibility issues between gems, it's actually impossible to use the very latest version of all your dependencies. If you look at the number of download counts for the last five versions of shoulda-matchers on Rubygems, you'll see the the distribution isn't what you'd expect given the frequency of releases. This is because people continue to download and use old versions.

To touch on the case of being able to have reproducible regressions, in reality, the only way to get full coverage is to run the tests against every permutation allowed by the gemspec.

To have absolute coverage, this is true. I think having a few reproducible scenarios is better than none, though, and given the fact that you can't actually prevent bugs because of the uncertainty of installed versions, I think this serves to decrease the importance of version regression testing.

What's even more important to me than having a slightly higher probability of catching a bug in the latest version is the ability to contribute. If somebody clones a repository, runs bundle install, and gets a bunch of random warnings or failures because they installed a combination nobody has noticed before, they're not going to contribute. They're going to (hopefully) file a bug report, and either work around the issue or remove the gem.

Keeping the lockfiles under version control gives you reproducible, reliable development environments. This not only increases the likelihood that you'll catch regressions, but it makes a sane development environment that will increase the number of contributors.

Contributor

sferik commented Mar 21, 2013

@jferris:

@sferik I think ignoring the gemfiles and version specifications are separate issues. I think the bourne dependency is better with a loose specification, although mocha (a dependency of bourne) doesn't follow SemVer, so it's not clear cut in this case.

I completely agree with your assessment. Using the pessimistic version constraint with two digits of precision only makes sense in cases where the dependent library:

  1. has a version number >= 1.0.0 and
  2. follows semantic versioning.

Neither of these hold are true for mocha, so it may make sense to specify a tighter constraint on mocha while specifying a looser constraint on shoulda-matchers.

It sounds like we are in complete agreement on this point.

drapergeek referenced this pull request in thoughtbot/shoulda-matchers Feb 9, 2014

Closed

Remove gemfiles lock #439

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