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

Remove temp build dir even if the req is not installed (fixes #420) #599

Merged
merged 7 commits into from
Jul 30, 2012
Merged

Remove temp build dir even if the req is not installed (fixes #420) #599

merged 7 commits into from
Jul 30, 2012

Conversation

claymation
Copy link
Contributor

This change ensures that RequirementSet.cleanup_files() is called whenever req_to_install._temp_build_dir is defined, preventing detritus from accumulating in $TMPDIR.

@travisbot
Copy link

This pull request fails (merged 48d1d87c into c6789f6).

@claymation
Copy link
Contributor Author

Note that the travisbot failure above seems unrelated to the changes in this pull request. It seems like an infrastructure failure. Can you try to build this pull request again?

@hltbra
Copy link
Contributor

hltbra commented Jul 18, 2012

Just to reinforce what @claymation says I got it from travis-ci:

Executing your before_install (sudo apt-get install subversion bzr mercurial)
took longer than 900 seconds and was terminated. Consider rewriting your stuff
in AssemblyScript, we've heard it handles Web Scale™

@qwcode
Copy link
Contributor

qwcode commented Jul 18, 2012

I think @pnasrat is the admin on our travis account, but I've seen other people make a trivial change (like change a commit timestamp or a commit comment), and then force push to re-trigger a build when it's obviously not the pull request's fault.


TestFileEnvironment provides a mechanism to ensure that
no temporary files are left after a test script executes,
so we can take advantage of that here by simply running
Copy link
Contributor

Choose a reason for hiding this comment

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

it could be helpful to mention that the mechanism is that ProcResult.assert_no_temp gets called and throws an exception if the tmp is non-empty.
that will make people feel better that this test can actually fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, sure, feel free to add the gory details if you'd like :)

Copy link
Contributor

Choose a reason for hiding this comment

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

the point is to be clear that the "mechanism" can actually cause a test failure, since you're not doing an explicit assert.

@claymation
Copy link
Contributor Author

Thanks, I'll force push to trigger a build.

@travisbot
Copy link

This pull request passes (merged 865af46 into c6789f6).

@qwcode
Copy link
Contributor

qwcode commented Jul 18, 2012

I want to merge this in, but I'm heading on a trip. I'll get back to this next week, unless somebody else merges first. I do want to fix that test.

@qwcode
Copy link
Contributor

qwcode commented Jul 18, 2012

btw, I did also manually verify the "pip-random-build" temp dirs are getting cleaned up in this branch for the scenario mentioned in #420.

pnasrat added a commit that referenced this pull request Jul 30, 2012
test enhancements to pull #599 (which fixes issue #420)
@pnasrat pnasrat merged commit 865af46 into pypa:develop Jul 30, 2012
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants