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

sequester tmp directories #1987

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@tduehr
Contributor

tduehr commented Sep 19, 2014

Moves tmp_* directories under tmp/.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 2, 2014

Member

Interesting idea. I feel a little weird about committing a tmp dir though. @enebo?

Member

headius commented Nov 2, 2014

Interesting idea. I feel a little weird about committing a tmp dir though. @enebo?

@tduehr

This comment has been minimized.

Show comment
Hide comment
@tduehr

tduehr Nov 3, 2014

Contributor

I'll take another look... I didn't look into generating the tmp dir as needed for some reason.

Contributor

tduehr commented Nov 3, 2014

I'll take another look... I didn't look into generating the tmp dir as needed for some reason.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 3, 2014

Member

I would almost prefer we just moved this to a build dir somewhere and probably leave them named as-is? Now that we use maven we no longer use build but having yet another scratch dir seems less appealing (even though it is more appealing than n tmp_* files).

Also what is that .gitignore line for?

Member

enebo commented Nov 3, 2014

I would almost prefer we just moved this to a build dir somewhere and probably leave them named as-is? Now that we use maven we no longer use build but having yet another scratch dir seems less appealing (even though it is more appealing than n tmp_* files).

Also what is that .gitignore line for?

@tduehr

This comment has been minimized.

Show comment
Hide comment
@tduehr

tduehr Nov 3, 2014

Contributor

the .gitignore is so that tmp dir gets saved to the repo. Moving the tmp_* directories to a build dir is equivalent for my purposes... I was just tired of having to manually delete them since they don't clean up properly with certain test failures.

Contributor

tduehr commented Nov 3, 2014

the .gitignore is so that tmp dir gets saved to the repo. Moving the tmp_* directories to a build dir is equivalent for my purposes... I was just tired of having to manually delete them since they don't clean up properly with certain test failures.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 3, 2014

Member

@tduehr Wow I did not notice that is tmp/.gitignore nor know you could have sub directory .gitignores!!!

Member

enebo commented Nov 3, 2014

@tduehr Wow I did not notice that is tmp/.gitignore nor know you could have sub directory .gitignores!!!

@tduehr

This comment has been minimized.

Show comment
Hide comment
@tduehr

tduehr Nov 3, 2014

Contributor

yup, the common idiom for keeping a directory around is either a .gitkeep file which does nothing but it's content and therefore can be tracked by git. Or add a .gitignore which has the added bonus of being able to keep the contents of the directory out of the repo without having to ignore the directory itself. Unsurprisingly .gitkeep has fallen out of favor.

Contributor

tduehr commented Nov 3, 2014

yup, the common idiom for keeping a directory around is either a .gitkeep file which does nothing but it's content and therefore can be tracked by git. Or add a .gitignore which has the added bonus of being able to keep the contents of the directory out of the repo without having to ignore the directory itself. Unsurprisingly .gitkeep has fallen out of favor.

@nirvdrum

This comment has been minimized.

Show comment
Hide comment
@nirvdrum

nirvdrum Mar 6, 2016

Contributor

Has there been any decision on this one?

Contributor

nirvdrum commented Mar 6, 2016

Has there been any decision on this one?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 2, 2016

Member

I still don't like the idea of having tmp committed to the repository with nothing but .gitignore in it. We should just fix the test to use a system-level temp location. That would be good enough, right?

Member

headius commented Nov 2, 2016

I still don't like the idea of having tmp committed to the repository with nothing but .gitignore in it. We should just fix the test to use a system-level temp location. That would be good enough, right?

@tduehr

This comment has been minimized.

Show comment
Hide comment
@tduehr

tduehr Nov 2, 2016

Contributor

I agree that would be better.

Contributor

tduehr commented Nov 2, 2016

I agree that would be better.

headius added a commit that referenced this pull request Nov 2, 2016

Fix problems with lingering temp dirs.
* Use system-level temp location from Dir.mktmpdir.
* Use FileUtils.rm_rf to delete directory in teardown.

See #1987.

@headius headius closed this Nov 2, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 2, 2016

Member

There was an additional problem with the test: it used rm_f instead of rm_rf to delete the @tmpdir in teardown. In other words, it didn't actually delete it. Both issues are fixed now.

Member

headius commented Nov 2, 2016

There was an additional problem with the test: it used rm_f instead of rm_rf to delete the @tmpdir in teardown. In other words, it didn't actually delete it. Both issues are fixed now.

@headius headius added this to the JRuby 9.1.6.0 milestone Nov 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment