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

runtests.pl: kill processes locking test log files #6179

Closed
wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Nov 5, 2020

For now only required and implemented for Windows.

Ref: #6058

@mback2k mback2k added tests Windows Windows-specific labels Nov 5, 2020
@mback2k mback2k self-assigned this Nov 5, 2020
@ghost
Copy link

ghost commented Nov 9, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.315 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

mback2k added a commit to mback2k/curl that referenced this pull request Nov 11, 2020
For now only required and implemented for Windows.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 11, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 11, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 14, 2020
For now only required and implemented for Windows.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 14, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 14, 2020
For now only required and implemented for Windows.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 14, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 15, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 15, 2020
For now only required and implemented for Windows.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 16, 2020
For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 20, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Nov 20, 2020
For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
@mback2k
Copy link
Member Author

mback2k commented Dec 5, 2020

I will continue working on this. I also plan to implement Linux/macOS support for this via lsof.

@bagder
Copy link
Member

bagder commented Dec 21, 2020

on Linux and macOS the files aren't "locked" when another process is reading/writing them so what would the reason and idea be for this on those platforms?

@mback2k
Copy link
Member Author

mback2k commented Dec 21, 2020

on Linux and macOS the files aren't "locked" when another process is reading/writing them so what would the reason and idea be for this on those platforms?

That is correct, but it would still be good to know if the files are kept open while they should already be closed. So the detection part is probably a good idea, but maybe not the process killing part.

mback2k added a commit to mback2k/curl that referenced this pull request Dec 26, 2020
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Dec 26, 2020
For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Feb 27, 2021
While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
mback2k added a commit to mback2k/curl that referenced this pull request Feb 27, 2021
For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Ref: curl#6058
Closes curl#6179
@mback2k
Copy link
Member Author

mback2k commented Feb 27, 2021

@bagder you were right, since the deletion of "locked" files won't fail, the detection part wouldn't even run like on Windows.

@mback2k mback2k marked this pull request as ready for review February 27, 2021 20:26
@mback2k
Copy link
Member Author

mback2k commented Feb 27, 2021

Rebased and ready for review.

tests/pathhelp.pm Show resolved Hide resolved
tests/runtests.pl Show resolved Hide resolved
tests/runtests.pl Show resolved Hide resolved
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

I think you should make this behavior an option, like runtests.pl -f forcibly kill lingering processes or something. This seems like a diagnostic for us to run in the CI and not for default use.

@mback2k
Copy link
Member Author

mback2k commented Feb 28, 2021

I consider it a general part for CI runs that should always be enabled, but yes: a flag for outside of CI makes sense.

mback2k added a commit to mback2k/curl that referenced this pull request Feb 28, 2021
For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Reviewed-by: Jay Satiro

Ref: curl#6058
Closes curl#6179
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

The commit message should say that this is a new option

While Msys2 has a pwd binary which supports -L,
Msys1 only has a built-in one with that feature.

Part of curl#6179
Introduce a new runtests.pl command option: -rm

For now only required and implemented for Windows.
Ignore stunnel logs due to long running processes.

Requires Sysinternals handle[64].exe to be on PATH.

Reviewed-by: Jay Satiro

Ref: curl#6058
Closes curl#6179
mback2k added a commit that referenced this pull request Mar 1, 2021
While Msys2 has a pwd binary which supports -L,
Msys1 only has a shell built-in with that feature.

Reviewed-by: Jay Satiro

Part of #6179
@mback2k mback2k closed this in 311c31e Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

3 participants