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

Use Bundler.with_original_env instead of custom clean environment logic #202

Merged

Conversation

kyrofa
Copy link
Contributor

@kyrofa kyrofa commented Jan 17, 2023

This is simply a version of #174 that:

  1. Is rebased on the most recent main
  2. Has tests that actually pass

(2) is done by way of introducing a new 'APPRAISAL_UNDER_TEST' environment variable that Command uses to determine if Appraisal itself is under test, and if so, tweaks the environment in the way the tests require.

@nickcharlton
Copy link
Member

Thanks for doing this, and sorry for taking so long to get around to looking at it. My plan is to merge this in the next few days (unfortunately, I've run out of time today).

One question, I'm getting this locally:

Failures:

  1) Appraisals file Bundler DSL compatibility supports all Bundler DSL in Appraisals file
     Failure/Error:
       raise RuntimeError, <<-error_message.strip_heredoc
         Command #{command.inspect} exited with status #{exitstatus}. Output:
         #{output.gsub(/^/, '  ')}
       error_message

     RuntimeError:
                 Command "bundle install --local" exited with status 17. Output:
                   Fetching ../gems/croissant
       Fetching ../gems/egg
       Fetching ../gems/omelette
     # ./spec/support/acceptance_test_helpers.rb:162:in `block (2 levels) in run'
     # <internal:kernel>:90:in `tap'
     # ./spec/support/acceptance_test_helpers.rb:154:in `block in run'
     # ./spec/support/acceptance_test_helpers.rb:149:in `chdir'
     # ./spec/support/acceptance_test_helpers.rb:149:in `in_test_directory'
     # ./spec/support/acceptance_test_helpers.rb:153:in `run'
     # ./spec/acceptance/appraisals_file_bundler_dsl_compatibility_spec.rb:92:in `block (2 levels) in <top (required)>'

Have you been seeing the same?

@nickcharlton
Copy link
Member

nickcharlton commented Jan 27, 2023

Oh! It's fixed with Bundler v2.3.7. That's acceptable, I think.

This fixes a long-running issue with more recent Bundler versions and
compatibility with GitHub Actions (and other CI services), where the
environment wasn't being handled correctly.

Introduce a new 'APPRAISAL_UNDER_TEST' environment variable that
`Command` uses to determine if Appraisal itself is under test, and if
so, tweaks the environment in the way the tests require. This allows the
tests to pass while using `Bundler.with_original_env`.

Fixes thoughtbot#173
@kyrofa
Copy link
Contributor Author

kyrofa commented Jan 27, 2023

I don't see it locally, using bundler v2.2.33 (which is what is stated in the lockfile). Note that bundler v2.3 and later, if combined with RubyGems v3.3 or later, automatically re-execs into the BUNDLED_WITH version from the lockfile, so that latest test run you just did probably used v2.2.33 as well.

We will need to deal with those deprecations, they will eventually bite us. That'll be another PR, though.

@nickcharlton nickcharlton force-pushed the feature/bundler-with-original-env branch from b120c8e to fd888c9 Compare January 27, 2023 17:34
@nickcharlton nickcharlton merged commit ea2baff into thoughtbot:main Jan 27, 2023
@kyrofa kyrofa deleted the feature/bundler-with-original-env branch January 27, 2023 17:36
@nickcharlton
Copy link
Member

Yeah — should be much easier now this problem is solved. It's been a real headache here, elsewhere just for me, let alone everyone else!

Thanks for opening this new PR with the working tests and getting in touch the other week. Thanks also to everyone else who helped with this issue over the last while!

@kyrofa
Copy link
Contributor Author

kyrofa commented Jan 27, 2023

My pleasure. As I mentioned in my email, I'm happy to help maintain this if you'd like more hands.

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.

3 participants