Invalid caching path with custom BUNDLE_GEMFILE

[artplan1]

Hello,
path to be cached is invalid if env variable BUNDLE_GEMFILE leads to subdirectory

example repo: https://github.com/ErwinM/acts_as_tenant/blob/master/.github/workflows/ci.yml

gemfiles that are used as matrix, gemfiles are located in gemfiles subdirectory. Bundler installs gems in ./gemfiles/vendor/bundle, but path to gems is hardcoded to ./vendor/bundle in cache function in ruby/setup-ruby.

example of installation in ./gemfiles/vendor/bundle - https://github.com/ErwinM/acts_as_tenant/runs/1551693162#step:4:156

As quick-fix I've set BUNDLE_PATH_RELATIVE_TO_CWD, now gems are installed in ./vendor/bundle - ErwinM/acts_as_tenant#245

But maybe it will be better to change ruby/setup-ruby not to use hardcoded path or set path_relative_to_cwd as bundler's config by default or add notice to readme

Thanks

[eregon]

Thanks for the report.
Right, I didn't know Bundler would interpret the path relative to the Gemfile.
I'll investigate locally and fix it.

I guess passing an absolute path is an option, but not so nice in the logs,
or cache BUNDLER_GEMFILE's directory + vendor/bundler instead,
or use that other Bundler option.

[deivid-rodriguez]

Hi!

I would like to figure out what bundler should do, and make it do the right thing, but I'm not totally sure at the moment. So I think I would recommend passing an absolute path, to make the action independent of future behaviour.

I always thought using a path relative to the Gemfile was weird and surprising, and the BUNDLER_PATH_RELATIVE_TO_CWD feature flag was added to fix a bug related to that.

However, recently I started to think that the current behavior might be more useful, so maybe it just needs some informative CLI message when these two paths differ and there could be confusion?

The reason I think the current behaviour might be more useful is that it allows you to keep an isolated location for each Gemfile. So for example, if you have a root Gemfile, and then gemfiles/rails_52/Gemfile and gemfiles/rails_60/Gemfile, each of them will install gems to their own location and not interfere with each other. As per how they can interfere, for example, I recall TravisCI auto-cleaning up unused gems before saving the cache. If the same location is shared, some jobs would be invalidating the cache from other jobs, since what's unused for one Gemfile, it's used for others.

[artplan1]

Hi, @deivid-rodriguez

install gems to their own location

so this would require setting custom installation folder for Gemfile if it's set using BUNDLE_GEMFILE? And folder should have name based on Gemfile.lock's SHA or name or something?

Does the cache key is generated based on Gemfile.lock? If it is then auto-cleaning up unused gems before saving the cache. If the same location is shared, some jobs would be invalidating the cache from other jobs shouldn't affect caches.

having path_relative_to_cwd = true set by default seems like easiest solution for me (I didn't dig action's code very deeply :D) that will not require any changes.

[deivid-rodriguez]

so this would require setting custom installation folder for Gemfile if it's set using BUNDLE_GEMFILE? And folder should have name based on Gemfile.lock's SHA or name or something?

What I've seen work best with current bundler's behaviour is to put each Gemifle on a separate folder, so instead of having gemfiles/rails_61.gemfile and gemfiles/rails_60.gemfile, use gemfiles/rails_61/Gemfile and gemfiles/rails_60/Gemfile. That way if you have BUNDLE_PATH=vendor/bundle configured, each of the gemfiles will get its own independent home for gems (gemfiles/rails_60/vendor/bundle and gemfiles/rails_61/vendor/bundle, instead of a shared gemfiles/vendor/bundle).

Does the cache key is generated based on Gemfile.lock? If it is then auto-cleaning up unused gems before saving the cache. If the same location is shared, some jobs would be invalidating the cache from other jobs shouldn't affect caches.

you're right. I do recall problems on TravisCI due to this, I guess it didn't generate cache keys based off the lockfile 🤷‍♂️.

Still, it seems like a problem for working locally, specially with future bundler versions, which are expected to install gems locally by default (like npm's node_modules) and to auto-cleanup unused gems by default after bundle install. it could create situations where you have a working bundle, the you switch to export BUNDLE_GEMFILE=my_alternative_gemfile to do some work, and when you go back to the standard Gemfile, your bundle no longer works because it's missing gems.

With the current "use the Gemfile folder as the base for relative paths", there's a way out of these conflicts like I explained before.

[eregon]

I used the absolute path approach (dc58db7, test: 0e7d36a), seems the easiest and most reliable.
Test run:

• first run: https://github.com/eregon/setup-ruby/runs/1591122706?check_suite_focus=true (53s)
• second run, uses cache: https://github.com/eregon/setup-ruby/runs/1591133488?check_suite_focus=true (8s)

[eregon]

Fixed in https://github.com/ruby/setup-ruby/releases/tag/v1.60.1, thanks for the report.

[eregon]

In CanCanCommunity/cancancan#682 I found that some gems might depend on the vendor folder to be a sibling of the gemfile.
In that case, using $PWD/vendor/bundle results in a RuboCop error:

$ Run bundle exec rubocop
Error: Unsupported Ruby version 2.1 found in `TargetRubyVersion` parameter (in vendor/bundle/ruby/2.4.0/gems/rainbow-3.0.0/.rubocop.yml). 2.1-compatible analysis was dropped after version 0.58.
Supported versions: 2.2, 2.3, 2.4, 2.5, 2.6
Error: Process completed with exit code 2.

Which can be solved by adapting the .rubocop.yml with:

diff --git a/.rubocop.yml b/.rubocop.yml
index 214673a..8f5637e 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -37,4 +37,5 @@ AllCops:
   TargetRubyVersion: 2.2.0
   Exclude:
     - 'gemfiles/**/*'
+    - 'vendor/**/*'
     - 'Appraisals'

But it would have worked without changing that if we use gemfiles/vendor/bundle like bundle config --local path vendor/bundle does.

[artplan1]

hi @eregon

I assume gemfiles/**/* can be removed from Exclude with your change

and vendor/**/* is excluded by default in rubocop
https://github.com/rubocop-hq/rubocop/blob/master/config/default.yml#L66

in cancancan AllCops is overriden

you can merge overriden config with rubocop's default (https://docs.rubocop.org/rubocop/0.88/configuration.html#merging-arrays-using-inherit_mode)
or remove Exclude completly

inherit_mode:
  merge:
    - Exclude

more here: rubocop/rubocop#288

[eregon]

Excluding gemfiles/ is needed because it would otherwise results in RuboCop failures, and those files are generated.
It is quite unfortunate that RuboCop does not default to merging excludes.

inherit_mode:
  merge:
    - Exclude

Seems much harder to understand, so not sure it's a good trade-off.

[artplan1]

Excluding gemfiles/ is needed because it would otherwise results in RuboCop failures

I thought it's excluded because gems were installed in this folder, relative to gemfiles in that folder 🤔 and rubocop checked all gems installed in that folder

[eregon]

I thought it's excluded because gems were installed in this folder, relative to gemfiles in that folder and rubocop checked all gems installed in that folder

I thought too initially, but it seems it's actually for both reasons.