open-130: allow override of the downloads cache location

I looked this changeset over -  sorry for the delay, I had more to comment on here than I expected for a seemingly simple change. 

Here are my thoughts:

- it would be better to not duplicate code for creating the installable cache directory if it doesn't exist
- the directory used will not be differentiated by user (unlike the current tmp cache dir). Was this intentional?
- what if the user sets AUTOBUILD_INSTALLABLE_CACHE to the empty string? to '/'? are there other values we should protect against?
- the method where this code is inserted is called "get_default_install_cache_dir" - this description is no longer true (it's not 'default') if we allow the user to configure the directory inside this method. The method should then either be renamed, or this code should reside elsewhere.

Finally, what are the usecases that this environment variable would address? Who will use it, and is there a problem that this fixes?

Oz Linden Jan. 23, 2012, 1:05 p.m. (Jan. 23, 2012, 1:05 p.m.)

It's not easy to avoid duplicating the code to create the directory if it doesn't exist, because the creation in the unconfigured case is down in the get_temp_dir function which the new code path does not use.  Doing it in this method is needed to preserve interface compatibility.

Whether or not the directory used is differentiated by user when the user has overridden it is, of course, up to the user who configured it.  I don't think that we need to worry about it (indeed, given the protections we have elsewhere around package file consistency, it could be argued that shared cache directories are just fine).

If the user configures a directory that can't be created, they'll get an error... again, if you configure it then it's up to you to make sure that what you configure is doable.  Trying to test for all possible failure cases isn't really practical.  If you think it's important, we could wrap the creation in a try block and put out an error message... but there are lots of other places where we don't do that, many of which could fail.

Yes, the method should have been renamed... will upload an additional patch to fix that.

The use cases are explained in the linked jira issue.


Submitter:	Oz Linden	Reviewers
Branch:		Groups:
Bugs:	open-130	People:	jenn
Change Number:	None	Repository:	autobuild

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11