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

[changelog skip] Fix \n in export path #965

Merged
merged 2 commits into from
Mar 17, 2020
Merged

Conversation

schneems
Copy link
Contributor

The command which returns a newline which was being put in the PATH and then exported to the export file:

which node: "/tmp/build_d21ec1d0a2e7e9466b95cd2f7f79b41a/.heroku/node/bin/node\n"

Would cause this to be in the PATH:

remote: export PATH=/tmp/build_449e75fdca9780bfa7f33fac3b86065e/./vendor/bundle/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/vendor/ruby-2.6.2/bin:/tmp/tmp.ly4bnGG8nm/bin/:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/node/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/node_modules/.bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/node/bin/node
remote: :/tmp/build_449e75fdca9780bfa7f33fac3b86065e/bin:/usr/local/bin:/usr/bin:/bin:$PATH

This causes the multi buildpack to fail #963, but only the multi buildpack. Error is:

remote: /tmp/buildpackYYwZE/export: line 2: :/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/bin:/usr/local/bin:/usr/bin:/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/./vendor/bundle/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/vendor/ruby-2.6.5/bin:/tmp/tmp.behzAImwsj/bin/:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/node/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/node_modules/.bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/node/bin/node: No such file or directory

Even though this is the bug, it has been in the ruby buildpack for some time. Something in #888 introduced a failure mode.

Since multi-buildpack is deprecated and unmaintained I don't want to add an automated test here. You can manually reproduce the failure by running:

cd /tmp
dir_name=$(ruby -e "require 'securerandom'; puts SecureRandom.hex")
mkdir $dir_name
cd $dir_name
echo "source 'https://rubygems.org'" > Gemfile
bundle install
git init .
git add .
git commit -m first
echo '{ "engines" : { "node": "8.9.4" } }' > package.json
heroku create
heroku buildpacks:set https://github.com/heroku/heroku-buildpack-multi
echo "https://github.com/heroku/heroku-buildpack-nodejs.git" >> .buildpacks
echo "https://github.com/heroku/heroku-buildpack-ruby" >> .buildpacks
git add . ; git commit -m next
git push heroku master

You can get it to pass by running

rm .buildpacks
echo "https://github.com/heroku/heroku-buildpack-nodejs.git" >> .buildpacks
echo "https://github.com/heroku/heroku-buildpack-ruby#schneems/fix-node-which" >> .buildpacks

Which points the app at this PR.

@edmorley
Copy link
Member

Out of curiosity I had a quick look through #888 but couldn't see an obvious cause there that exposed this. Guess somewhat irrelevant given this was a real bug anyway :-)

@schneems
Copy link
Contributor Author

schneems commented Mar 17, 2020

Here's the export file before and after #888

Before #888

export GEM_PATH="/tmp/build_7150eb1767e7c0074cc51707f27954f0/vendor/bundle/ruby/2.6.0:$GEM_PATH"
export LANG=${LANG:-en_US.UTF-8}
export PATH="/tmp/build_7150eb1767e7c0074cc51707f27954f0/vendor/bundle/bin:/tmp/build_7150eb1767e7c0074cc51707f27954f0/vendor/bundle/ruby/2.6.0/bin:/tmp/build_7150eb1767e7c0074cc51707f27954f0/vendor/ruby-2.6.5/bin:/tmp/tmp.FwT0R4i7tn/bin/:/tmp/build_7150eb1767e7c0074cc51707f27954f0/.heroku/node/bin:/tmp/build_7150eb1767e7c0074cc51707f27954f0/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_7150eb1767e7c0074cc51707f27954f0/node_modules/.bin:/tmp/build_7150eb1767e7c0074cc51707f27954f0/.heroku/node/bin/node
:/tmp/build_7150eb1767e7c0074cc51707f27954f0/bin:/usr/local/bin:/usr/bin:/bin"

After #888

export PATH=/tmp/build_4ade224d4cac2630313b2c6ab0420e94/./vendor/bundle/bin:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/vendor/ruby-2.6.5/bin:/tmp/tmp.agFPALxbad/bin/:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/.heroku/node/bin:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/node_modules/.bin:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/.heroku/node/bin/node
:/tmp/build_4ade224d4cac2630313b2c6ab0420e94/bin:/usr/local/bin:/usr/bin:/bin:$PATH
export GEM_PATH=/tmp/build_4ade224d4cac2630313b2c6ab0420e94/vendor/bundle/ruby/2.6.0:$GEM_PATH
export LANG=${LANG:-en_US.UTF-8}

While this PR fixes the immediate issue. The problem seems to be that the export file values were being quoted, but that functionality was quietly dropped and not caught by the tests.

I'm not sure why heroku buildpacks works but the multibuildpack does not, i'm guessing it's a difference between how the file is exec-d or parsed. I think we need to make sure values are quoted before we can deploy. Merging this PR in as an immediate fix

@schneems schneems changed the title Fix \n in export path [changelog skip] Fix \n in export path Mar 17, 2020
The command `which` returns a newline which was being put in the PATH and then exported to the export file:

```
which node: "/tmp/build_d21ec1d0a2e7e9466b95cd2f7f79b41a/.heroku/node/bin/node\n"
```

Would cause this to be in the PATH:

```
remote: export PATH=/tmp/build_449e75fdca9780bfa7f33fac3b86065e/./vendor/bundle/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/vendor/ruby-2.6.2/bin:/tmp/tmp.ly4bnGG8nm/bin/:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/node/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/node_modules/.bin:/tmp/build_449e75fdca9780bfa7f33fac3b86065e/.heroku/node/bin/node
remote: :/tmp/build_449e75fdca9780bfa7f33fac3b86065e/bin:/usr/local/bin:/usr/bin:/bin:$PATH
```

This causes the multi buildpack to fail #963, but only the multi buildpack. Error is:

```
remote: /tmp/buildpackYYwZE/export: line 2: :/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/bin:/usr/local/bin:/usr/bin:/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/./vendor/bundle/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/vendor/ruby-2.6.5/bin:/tmp/tmp.behzAImwsj/bin/:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/node/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/yarn/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/node_modules/.bin:/tmp/build_71238ae8789a5e65c5afca4e8abc6c3a/.heroku/node/bin/node: No such file or directory
```

Even though this is the bug, it has been in the ruby buildpack for some time. Something in #888 introduced a failure mode.

Since multi-buildpack is deprecated and unmaintained I don't want to add an automated test here. You can manually reproduce the failure by running:

```
cd /tmp
dir_name=$(ruby -e "require 'securerandom'; puts SecureRandom.hex")
mkdir $dir_name
cd $dir_name
echo "source 'https://rubygems.org'" > Gemfile
bundle install
git init .
git add .
git commit -m first
echo '{ "engines" : { "node": "8.9.4" } }' > package.json
heroku create
heroku buildpacks:set https://github.com/heroku/heroku-buildpack-multi
echo "https://github.com/heroku/heroku-buildpack-nodejs.git" >> .buildpacks
echo "https://github.com/heroku/heroku-buildpack-ruby" >> .buildpacks
git add . ; git commit -m next
git push heroku master
```

You can get it to pass by running

```
rm .buildpacks
echo "https://github.com/heroku/heroku-buildpack-nodejs.git" >> .buildpacks
echo "https://github.com/heroku/heroku-buildpack-ruby#schneems/fix-node-which" >> .buildpacks
```

Which points the app at this PR.
@schneems
Copy link
Contributor Author

I've got a PR with a test and a fix to the described export quoting issue in #967

schneems added a commit that referenced this pull request Mar 18, 2020
This is a followup from #965.

In #888 the values in the buildpack's `export` file dropped their quoting for paths causing a few failures that were not caught by our tests.

This PR fixes the underlying problem and adds a test that would have caught the regression. The test uses a buildpack that intentionally adds a quoted newline to it's export file https://github.com/sharpstone/export_path_with_newlines_buildpack/blob/master/bin/compile this gets picked up by the Ruby buildpack and put back into Ruby's `export` file. Since ruby after #888 did not quote these values it would result in this failure:

```
  1) Buildpack internals handles PATH with a newline in it correctly
     Failure/Error:
       Hatchet::Runner.new("default_ruby", buildpacks: buildpacks).deploy do |app|
         expect(app.output).to_not match("No such file or directory")
       end
     Hatchet::App::FailedDeploy:
       Could not deploy 'hatchet-t-3e6bacb18a' (default_ruby) using 'Hatchet::GitApp' at path: './repos/rack/default_ruby'
        if this was expected add `allow_failure: true` to your deploy hash.
       output:
       Buildpack: nil
       Repo: https://git.heroku.com/hatchet-t-3e6bacb18a.git
       remote: Compressing source files... done.        
       remote: Building source:        
       remote: 
       remote: -----> I add newlines to the export file app detected        
       remote: -----> Ruby app detected        
       remote: -----> Installing bundler 1.17.3        
       remote: -----> Removing BUNDLED WITH version in the Gemfile.lock        
       remote: -----> Compiling Ruby/Rack        
       remote: -----> Using Ruby version: ruby-2.6.5        
       remote: -----> Installing dependencies using bundler 1.17.3        
       remote:        Running: bundle install --without development:test --path vendor/bundle --binstubs vendor/bundle/bin -j4 --deployment        
       remote:        Fetching gem metadata from https://rubygems.org/...............        
       remote:        Using bundler 1.17.3        
       remote:        Fetching rack 1.5.0        
       remote:        Installing rack 1.5.0        
       remote:        Bundle complete! 1 Gemfile dependency, 2 gems now installed.        
       remote:        Gems in the groups development and test were not installed.        
       remote:        Bundled gems are installed into `./vendor/bundle`        
       remote:        Bundle completed (3.14s)        
       remote:        Cleaning up the bundler cache.        
       remote: -----> Writing config/database.yml to read from DATABASE_URL        
       remote: -----> Detecting rake tasks        
       remote: 
       remote: ###### WARNING:        
       remote: 
       remote:        You have not declared a Ruby version in your Gemfile.        
       remote:        To set your Ruby version add this line to your Gemfile:        
       remote:        ruby '2.6.5'        
       remote:        # See https://devcenter.heroku.com/articles/ruby-versions for more information.        
       remote: 
       remote: ###### WARNING:        
       remote: 
       remote:        No Procfile detected, using the default web server.        
       remote:        We recommend explicitly declaring how to boot your server process via a Procfile.        
       remote:        https://devcenter.heroku.com/articles/ruby-default-web-server        
       remote: 
       remote: 
       remote: -----> Null Buildpack app detected        
       remote: /app/tmp/buildpacks/6ccd2b9f41db985dbeffeeafa11877f50b22197988b0d84a055bdf7fe48efeff0c1fbccefc0b0ef5dd833ede16523bd9cec929eeebc3e0e3bfe67c0f12460db2/export: line 2: /dog:/tmp/build_fdb4f5eaca8a5eb93660ff3af68703df/hello/there:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/tmp/build_fdb4f5eaca8a5eb93660ff3af68703df/bin:/usr/local/bin:/usr/bin:/bin:/tmp/build_fdb4f5eaca8a5eb93660ff3af68703df/./vendor/bundle/bin:/tmp/build_fdb4f5eaca8a5eb93660ff3af68703df/./vendor/bundle/ruby/2.6.0/bin:/tmp/build_fdb4f5eaca8a5eb93660ff3af68703df/vendor/ruby-2.6.5/bin:/tmp/tmp.HN41keQoC9/bin/:cinco/: No such file or directory        
       remote: /usr/bin/env: ‘bash’: No such file or directory        
       remote:  !     Push rejected, failed to compile Null Buildpack app.        
       remote: 
       remote:  !     Push failed        
       remote: Verifying deploy...        
       remote: 
       remote: !	Push rejected to hatchet-t-3e6bacb18a.        
       remote: 

```
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.

None yet

4 participants