Skip to content

use exec to run tailwind binary, so return codes pass through #181

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

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

blerchin
Copy link
Contributor

@blerchin blerchin commented Jul 6, 2022

We are encountering issues where our CI pipeline can succeed without successfully building Tailwind. It appears this is possible because the TAILWIND_COMPILE_COMMAND is run with system and the return code is never checked. When problems occur, an error is printed to the console but the return code from running rails tailwindcss:build is still 0.

Build errors, such as those caused by a version mismatch between the tailwindcss-rails and the corresponding binary gem, could cause a CI build to fail silently and lead to an unstyled site in production. Therefore I believe it is essential that these rake tasks pass through the return code.

Using Kernel#exec replaces the current process by running the given external command, thereby passing through any return codes. If this is unacceptable, another solution would be to add the exception: true flag to the system call.

@rafaelfranca rafaelfranca merged commit 2e7ee0e into rails:main Jul 6, 2022
@blerchin blerchin deleted the fail-on-build-errors branch July 6, 2022 23:10
@flavorjones
Copy link
Member

I just want to note that there's no test coverage on these rake tasks, and I have mild concerns that exec behaves differently on windows than it will on posix-y systems, see for example rubys/sprockets-esbuild#4

If someone can give this a whirl on a Windows machine and comment here I'd feel much more comfortable about it. Or, probably preferably, we should actually exercise these rake tests in the test suite, and I apologize that I don't have the bandwidth to do that work right now.

@blerchin
Copy link
Contributor Author

blerchin commented Jul 7, 2022

I just want to note that there's no test coverage on these rake tasks, and I have mild concerns that exec behaves differently on windows than it will on posix-y systems, see for example rubys/sprockets-esbuild#4

If someone can give this a whirl on a Windows machine and comment here I'd feel much more comfortable about it. Or, probably preferably, we should actually exercise these rake tests in the test suite, and I apologize that I don't have the bandwidth to do that work right now.

That's a fair point, I think I have a windows box I could boot up to confirm this doesn't break anything. As far as testing goes, I'm not sure how valuable it would be unless we have a pipeline to run tests on multiple platforms... these are one-liner rake tasks so we'd basically be testing the Kernel implementation.

@blerchin
Copy link
Contributor Author

blerchin commented Jul 8, 2022

Confirmed that there is no change in behavior when using exec in the rake task.
This is running via cmd with Windows 10.0.19041, Ruby 3.1.2(x64-mingw-ucrt)

@flavorjones
Copy link
Member

👍 Thank you for checking!

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.

3 participants