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

Fail ember:compile if build subprocess fails #418

Closed
wants to merge 2 commits into from
Closed

Conversation

seanpdoyle
Copy link
Contributor

Closes #417.

The following is the command ember-cli-rails generates for rake ember:compile. It was generated using the example application.

/Users/seanpdoyle/src/ember-cli-rails-heroku-example/frontend/node_modules/.bin/ember build --environment 'development' --output-path '/Users/seanpdoyle/src/ember-cli-rails-heroku-example/tmp/ember-cli/apps/frontend' | /usr/bin/tee -a '/Users/seanpdoyle/src/ember-cli-rails-heroku-example/log/ember-frontend.development.log'

Pipes are streaming, so the pipe to tee will open before the first
program finishes writing, potentially ignoring a non-zero exit status.

The solution is to set the pipefail option for the generated
sub-command.

paths = build_paths(tee: "path/to/tee", log: "path/to/log")
command = build_command(paths: paths)

expect(command.build).to match(%r{\Aset -o pipefail;})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use String#start_with? instead of a regex match anchored to the beginning of the string.

@jferris
Copy link

jferris commented Mar 17, 2016

I think you forgot to close your code block.

Closes [#417][#417].

The following is the command `ember-cli-rails` generates for `rake
ember:compile`. It was generated using [the example application][repo].

```bash
/Users/seanpdoyle/src/ember-cli-rails-heroku-example/frontend/node_modules/.bin/ember build --environment 'development' --output-path '/Users/seanpdoyle/src/ember-cli-rails-heroku-example/tmp/ember-cli/apps/frontend' | /usr/bin/tee -a '/Users/seanpdoyle/src/ember-cli-rails-heroku-example/log/ember-frontend.development.log'
```

Pipes are streaming, so the pipe to `tee` will open before the first
program finishes writing, potentially ignoring a non-zero exit status.

The solution is to set the [`pipefail`][docs] option for the generated
sub-command.

[repo]: https://github.com/seanpdoyle/ember-cli-rails-heroku-example
[#417]: #417
[docs]: http://www.gnu.org/software/bash/manual/html_node/Pipelines.html
@seanpdoyle
Copy link
Contributor Author

I think you forgot to close your code block.

I was frantically searching the code diff for a Ruby syntax error. 😶

I've corrected the commit message.

@mike-burns
Copy link

I'm not familiar with this project so I don't know the details, but I did get here via an appeal to tbot/shell.

If that string is to be read by a POSIX shell, you might want to know that:

$ set -o pipefail
sh: set: pipefail: bad option

If it's to be read by zsh, though, that will be fine.

If you want something that works from the shell, you could get fancy with a pipe (inspired by StackOverflow here):

$ mkfifo pipe
$ tee out.txt < pipe &
[1] 23301
$ (echo tada ; false) > pipe
$ 
[1] + Done                 tee out.txt < pipe 
$ echo $?
1
$ cat out.txt
tada

@seanpdoyle
Copy link
Contributor Author

ERROR: Failed command: `set -o pipefail; /home/travis/build/thoughtbot/ember-cli-rails/spec/dummy/my-app/node_modules/ember-cli/bin/ember build --environment 'development' --output-path '/home/travis/build/thoughtbot/ember-cli-rails/spec/dummy/tmp/ember-cli/apps/my-app' | /usr/bin/tee -a '/home/travis/build/thoughtbot/ember-cli-rails/spec/dummy/log/ember-my-app.test.log'`
OUTPUT:
  sh: 1: set: Illegal option -o pipefail

:sad-trombone: @mike-burns would set -e; instead of set -o pipefail be somewhat equivalent behavior? Is set -e a POSIX-compliant command?

@mike-burns
Copy link

$ set -e
$ false | tee
$ echo $?
0

Doesn't seem to do what you want.

@klaustopher
Copy link

With the current version on this branch (b0ba573) the problem is back...

$ bin/rake ember:compile --trace
** Invoke ember:compile (first_time)
** Invoke ember:install (first_time)
** Invoke environment (first_time)
** Execute environment
** Execute ember:install
** Execute ember:compile

$ echo $?
0

$ cat log/ember-demandcheck.development.log
/Users/klaustopher/Projects/application/demandcheck/node_modules/configstore/index.js:53
                throw err;
                ^

Error: EACCES: permission denied, open '/Users/klaustopher/.config/configstore/ember-cli.json'
You don't have access to this file.

    at Error (native)
    at Object.fs.openSync (fs.js:584:18)
    at Object.fs.readFileSync (fs.js:431:33)
    at Object.create.all.get (/Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/configstore/index.js:34:26)
    at Object.Configstore (/Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/configstore/index.js:27:44)
    at clientId (/Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/ember-cli/lib/cli/index.js:22:21)
    at module.exports (/Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/ember-cli/lib/cli/index.js:65:19)
    at /Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/ember-cli/bin/ember:26:3
    at /Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/resolve/lib/async.js:44:21
    at ondir (/Users/klaustopher/Projects/Clark/application/demandcheck/node_modules/resolve/lib/async.js:187:31)

@klaustopher
Copy link

Just a suggestion: Why are you using tee at all?

You capture the whole output with Open3.capture2eanyways into the output variable. You could than just write this output to a file. This would also have the benefit, that those commands that are not run through an EmberCli::Command like the npm and bower install's output would be written to a log file.

@seanpdoyle
Copy link
Contributor Author

Closing in favor of #420.

@seanpdoyle seanpdoyle closed this Mar 18, 2016
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.

5 participants