Allow configuration of vendor path

[pointlessone]

Currently bundler-cache option implies that gems will be installed in vendor/bundle in the source tree root. This is undesirable as it might interfere with tooling setup that can mistake vendored gems source for the project source.

Please allow configuration of the vendor path or at least provide an option to install gems outside of the project source tree.

[eregon]

Context: prawnpdf/prawn#1190

I think vendor/bundle is by very far the most common value for the Bundler path config.
In fact, bundle install --deployment actually sets path to vendor/bundle.

FWIW, TravisCI also uses vendor/bundle by default, but it seems to be somewhat configurable: https://docs.travis-ci.com/user/caching/#bundler

While I understand adding new files under the repository root might be undesirable, it also seems a very common and accepted practice. node.js has node_modules/ for instance.

So I think in general it's valuable to support bundle config --local path vendor/bundle for all gems, and in fact this is often something I do, e.g., when testing a gem with TruffleRuby, so the gems are installed under the project directory, and after rebuilding TruffleRuby the gems are still there.

I understand the request, but I question the need, is there an actual issue with having gems under vendor/bundle, which is the most common Bundler path?

RuboCop can be a little bit confusing, but I argue that's due to a bug of RuboCop (rubocop/rubocop#9325).
RuboCop does exclude vendor/**/* by default.
However, Exclude: is not yet merged by default in RuboCop, which I think is the real issue here: rubocop/rubocop#288 (comment)
A workaround is to make it explicit:

inherit_mode:
  merge:
    - Exclude

(https://docs.rubocop.org/rubocop/0.88/configuration.html#merging-arrays-using-inherit_mode)

Related: #131 (comment) (cc @artplan1).

---

Another issue if we would implement this is with self-hosted runners. On those runners, only the working directory is cleaned between builds. Which means if the Bundler path was set to e.g. ~/gems, then the gems would still be there on the next run and that could lead to confusing errors, e.g., if some gem failed to install, or if bundle clean is used.

[eregon]

I filed an issue to RuboCop because I think merging Exclude by default is the best solution here, and the most intuitive for RuboCop users: rubocop/rubocop#9325

[pointlessone]

The issue currently arises from RuboCop. However, I don't believe it is strictly a RuboCop issue. Pretty much any tool can be confused by extraneous files. For example, RSpec can easily pick up specs vendored specs with non-default spec pattern.

Custom operations like packaging might not expect those extra files as well. Those operation might result in incorrect artifacts.

In addition, in the context of CI (which I believe it the primary use of this action) bundle install --deployment is likely not the right thing to do and as such is probably not an argument for the current implementation.

[eregon]

For example, RSpec can easily pick up specs vendored specs with non-default spec pattern.

That's fairly rare though, isn't it? I guess 99% users put specs under spec/.
And even if there is a spec pattern, it should use some directory in the pattern, not just any file under the repository root.

Custom operations like packaging might not expect those extra files as well. Those operation might result in incorrect artifacts.

Packaging should always use an explicit include list (or git ls-files, which is a sort of include list).
Otherwise, it might include test fixtures, dotfiles (e.g., from the IDE, from bundle config, from the OS, ...), etc.

IDE files (and also directories, e.g., .idea) and .bundle/config are typical examples of extra files in the source directory, and yet it seems all tools can handle them fine. vendor/bundle only differs in containing a bit more files, and specifically Ruby files.

In addition, in the context of CI (which I believe it the primary use of this action) bundle install --deployment is likely not the right thing to do and as such is probably not an argument for the current implementation.

The deployment Bundler flag is actually set when there is a corresponding lockfile checked in the repository, since #53.
Note https://bundler.io/v2.0/man/bundle-install.1.html#DEPLOYMENT-MODE which says:

Bundler's defaults are optimized for development. To switch to defaults optimized for deployment and for CI, use the --deployment flag.

So the deployment flag is actually recommended for CI, according to Bundler docs. It also seems a common practice.

---

I think currently adding the complexity (harder to test, makes the key more complex, harder to reason where the files are and how long they are preserved, if they might collide with something else, etc) and risk (related to self-hosted runners as mentioned above) of a custom Bundler path is not worth it for what seems to be mostly personal preferences.

I'd like to suggest merging prawnpdf/prawn#1190 and reconsider if there is any issue. Of course, you can also decline it, but be aware that manually caching gems is typically incorrect (https://github.com/ruby/setup-ruby#caching-bundle-install-manually, prawnpdf/prawn#1190 (comment)).

[pointlessone]

That's fairly rare though, isn't it?

It is but I'm not arguing for changing the defaults but adding some flexibility to accommodate those rare cases.

I totally understand unwillingness to make things more complex than they need to be. After all, this is your project. To me it doesn't look like it would complicate things a lot. After all the vendor path is already in a variable. It would only take exposing an option for that variable to read from. Of course, I'm not aware of all the nuances of the action, one of which you've mentioned above.

Feel free to close this issue if you believe the feature doesn't fit the scope of the project.

[eregon]

I'm not so keen to add an extra input for this, I'd rather keep inputs to the strict minimum.
But, much like other Bundler settings can be set with BUNDLE_* env vars, there is BUNDLE_PATH, which the action could read (e.g., the action already reads BUNDLE_GEMFILE).
We'd need to make the path part of the key, so that if the path changes we don't attempt to reuse a previous cache.

[eregon]

I think allowing to customize the Bundler path would be a source of bugs and a lot of maintenance overhead, so I'm not willing to allow changing it currently.
My main concern is if the Bundler path is outside the working directly, it's much harder to reason about it: we don't know if that path will be cleaned correctly (especially the case for self-hosted runners), if it might be accessed by something else and the next build would see some dirty state, etc.
I might reconsider if it's really problematic or there is a significant advantage to being able to change the path (the RuboCop issue is not enough).