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

tools,makefile: fix file open mode and cleanup tap file #2837

Closed
wants to merge 2 commits into from

Conversation

thefourtheye
Copy link
Contributor

1st commit:

By default the logfile is opened in append mode. This commit makes sure
that the file is opened in write-binary mode, so that the file will be
created if it doesn't exist or overwrite if it exists.

2nd commit:

Make make clean cleanup the generated tap file as well.

Fixes: #2834

cc @nodejs/build

@thefourtheye thefourtheye added the tools Issues and PRs related to the tools directory. label Sep 12, 2015
@jbergstroem
Copy link
Member

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

@bnoordhuis
Copy link
Member

...with a teeny nit about the commit logs: s/Open/open/ and s/Makefile/build/ for consistency.

By default the logfile is opened in append mode. This commit makes sure
that the file is opened in write-binary mode, so that the file will be
created if it doesn't exist or overwrite if it exists.

Fixes: nodejs#2834
Make `make clean` cleanup the generated tap file as well.

Fixes: nodejs#2834
@thefourtheye thefourtheye added the build Issues and PRs related to build files or the CI. label Sep 13, 2015
@rvagg
Copy link
Member

rvagg commented Sep 14, 2015

lgtm, thanks for doing this @thefourtheye

@orangemocha
Copy link
Contributor

I wonder: does appending to a .tap file ever make sense? If not, it would be better to have the test runner delete the file (instead of make test-ci).

@thefourtheye
Copy link
Contributor Author

@orangemocha If test-runner deletes the file, then we will not get to see the TAP results, right?

@evanlucas
Copy link
Contributor

Maybe delete before running the tests?

@thefourtheye
Copy link
Contributor Author

@evanlucas This patch does something similar. Opens the file in write mode, which truncates the file if it exists.

@orangemocha
Copy link
Contributor

@thefourtheye : sorry, I didn't catch that before. Your first commit already does what I was suggesting :)

LGTM

thefourtheye added a commit that referenced this pull request Sep 15, 2015
By default the logfile is opened in append mode. This commit makes sure
that the file is opened in write-binary mode, so that the file will be
created if it doesn't exist or overwrite if it exists.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
thefourtheye added a commit that referenced this pull request Sep 15, 2015
Make `make clean` cleanup the generated tap file as well.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@thefourtheye
Copy link
Contributor Author

Thanks for the review people :-) Landed in 467eff2 and 91e7e9c

@thefourtheye thefourtheye deleted the fix-for-2834 branch September 15, 2015 04:51
thefourtheye added a commit that referenced this pull request Sep 15, 2015
By default the logfile is opened in append mode. This commit makes sure
that the file is opened in write-binary mode, so that the file will be
created if it doesn't exist or overwrite if it exists.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
thefourtheye added a commit that referenced this pull request Sep 15, 2015
Make `make clean` cleanup the generated tap file as well.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@rvagg rvagg mentioned this pull request Sep 15, 2015
@rvagg
Copy link
Member

rvagg commented Sep 15, 2015

@thefourtheye I'm just noticing that these two commits made it in with PR-URL: #2837 and Fixes: #2834, this is probably a simple oversight but please be sure to include full URLs in future to make accounting easier

@thefourtheye
Copy link
Contributor Author

@rvagg Ah, sorry about that. I just used that GitHub format. I'll keep it in mind for the next time.

@jbergstroem
Copy link
Member

@thefourtheye downside being very hard to click in terminal :)

thefourtheye added a commit that referenced this pull request Sep 15, 2015
By default the logfile is opened in append mode. This commit makes sure
that the file is opened in write-binary mode, so that the file will be
created if it doesn't exist or overwrite if it exists.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
thefourtheye added a commit that referenced this pull request Sep 15, 2015
Make `make clean` cleanup the generated tap file as well.

Fixes: #2834

PR-URL: #2837
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Fishrock123 Fishrock123 mentioned this pull request Sep 15, 2015
7 tasks
@rvagg rvagg mentioned this pull request Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants