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

u3d/install: allow to specify packages download directory using an environment variable #214

Merged

Conversation

niezbop
Copy link
Member

@niezbop niezbop commented Jan 6, 2018

This adds a new option to u3d install so that the user can specify the target of its destination.
u3d install <SOME STUFF> --download_path <SOME PATH> will download if not already present at specified path and install it from this file.
u3d install <SOME STUFF> --no-install --download_path <SOME PATH> will download packages at some location, and make sure that they don't already exist there.

This works for both absolute and relative paths.

@lacostej
Copy link
Member

lacostej commented Jan 6, 2018

I was thinking of doing this today, but with the use of a simple environment variable instead. That way the change had a much smaller footprint.

If we keep it as is, an alternative would be to use the u3d_core/globals to store the value instead of passing it to all methods down the call chain.

Together with adding a class method at downloader level

def self.download_dir
  # today's implementation
  # File.join(DOWNLOAD_PATH, DOWNLOAD_DIRECTORY
  # new implementation something like
  # to be discussed
  U3dCore::Globals.download_dir || ENV['U3D_DOWNLOAD_DIR'] || File.join(DOWNLOAD_PATH, DOWNLOAD_DIRECTORY)
end

the change becomes smaller. WDYT?

@niezbop
Copy link
Member Author

niezbop commented Jan 6, 2018

I agree that it is better as my change was introducing lots of API modifications that I wasn't peculiarly happy about. I believe that using environment variables makes sense for this feature but we should be careful not too introduce too many of those. But once again, it makes sense in this case, so I will do the modifications asap (not on my computer right now).

@niezbop niezbop force-pushed the downloader/add_download_path_option branch from c609428 to 571559f Compare January 6, 2018 22:25

The default download path is $HOME/Downloads/Unity_Packages/, but you may change that by specifying the environment variable U3D_DOWNLOAD_PATH.

E.g. U3D_RULES_PATH=/some/path/you/want u3d install ...
Copy link
Member

Choose a reason for hiding this comment

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

U3D_DOWNLOAD_PATH

@@ -53,6 +53,11 @@ def with_env_values(hash)
end
end

def warn_if_env(variable)
Copy link
Member

@lacostej lacostej Jan 7, 2018

Choose a reason for hiding this comment

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

should we fail instead? Should we fail the whole setup if the value is already set? (It might be useful for self-testing environment though).

@@ -386,7 +386,8 @@
end

describe '.destination_for' do
it 'returns the correct destination the Unity installer' do
it 'returns the correct default destination for the Unity installer' do
warn_if_env('U3D_DOWNLOAD_PATH')
Copy link
Member

@lacostej lacostej Jan 7, 2018

Choose a reason for hiding this comment

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

or should we call the test with

with_env_values('U3D_DOWNLOAD_PATH' => nil) do

instead?

Copy link
Member

@lacostej lacostej left a comment

Choose a reason for hiding this comment

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

Fine!

@lacostej lacostej changed the title u3d/downloader: add download path option u3d/downloader: allow to specify packages download directory using an environment variable Jan 8, 2018
@lacostej lacostej changed the title u3d/downloader: allow to specify packages download directory using an environment variable u3d/install: allow to specify packages download directory using an environment variable Jan 8, 2018
@niezbop niezbop merged commit 8de7421 into DragonBox:master Jan 8, 2018
@niezbop niezbop deleted the downloader/add_download_path_option branch January 8, 2018 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants