projectblacklight / blacklight

Blacklight provides a discovery interface for any Solr (http://lucene.apache.org/solr) index.
http://projectblacklight.org/
Other
757 stars 257 forks source link

Avoid double-escape in blacklight:assets generator with revert of shell escaping f491661 #3088

Closed jrochkind closed 12 months ago

jrochkind commented 12 months ago

Commit f491661 adds some Shellwords.escape calls to arguments to generate method, to call a the blacklight assets install generator from inside the main assets generator.

https://github.com/projectblacklight/blacklight/commit/f491661ad0a7e105c80e66a7625725c38072dff8#diff-c9171dc6ee82f6b53296c26439fe17f5f3fb38ac0db9aeedab740679dcbc1989R10

The commit message says this was necessary:

So that a string like ~> 4.0 can be passed without bash transforming ~ to $HOME"

But this is actually causing "double escaping" or "extraneous slashes" issues in some cases.

I ran into this problem in CI failures in blacklight_range_limit., such as you can see here:

https://github.com/projectblacklight/blacklight_range_limit/actions/runs/5834406502/job/15823853679#step:5:1088

After some effort, I am able to reproduce simply, especially in older Rails (that are still supported by Blacklight 8).

I am not able to reproduce the problem that @jcoyne said he was fixing in f491661

In this branch (with escaping removed), In MacOS bash 5.1: I am able to run: eg rails g blacklight:install --bootstrap-version="~> 5.2". Nothing gets interpreted as "home" alias. gem "bootstrap", "~> 5.2" is added to Gemfile.

So this is a revert of f491661 -- I am not able to reproduce the problem it was meant to solve, and reverting it resolves other problems I am able to reproduce.

@jcoyne , thoughts? Do you know what shell/version you were experiencing the problem in, which f491661 fixed? If it were necessary to fix a problem in some other shell, but causes a problem in Bash 5... that's a challenge!

jrochkind commented 12 months ago

All actual tests are passing -- the LINTER is failing.. on lines I do not touch in this PR.

Advice on what to do about that? I have shaved a lot of yaks today, and don't have time for this one.

Note the actual test runs are passing, it is only the linter which is failing, and on lines I didn't touch in this PR, which I don't understand.

jcoyne commented 12 months ago

@jrochkind this PR fixes the lint errors: https://github.com/projectblacklight/blacklight/pull/3087 I'd suggest that someone should merge that and this PR should be rebased.

jrochkind commented 12 months ago

merged and rebased!

jrochkind commented 12 months ago

Some tests failed this time, have no idea why, doesn't seem related to my change either?

Are there flakey tests? Trying to re-run failed tests.

jrochkind commented 12 months ago

There ARE flakey tests, re-running tests with no code changes has all green currently.

jcoyne commented 12 months ago

I can't remember why (in which environments) this was needed.

jrochkind commented 12 months ago

Thanks @jcoyne !