u3d/install: do not reinstall already installed packages #202

niezbop · 2017-12-26T01:53:13Z

Fixes #198

This solve the issues of non-matching package name between installed and available by:

Mapping the names of the installed package and the available package
Detecting some folders on the local install for "special package" (Documentation, Standard Assets...)

niezbop · 2017-12-26T01:53:28Z

This is still [WIP] because:

Example package is still not detected as it is not installed in the install but in the public home (ie C:\Users\Public\Documents\Unity Projects\Standard Assets Example Project on Windows), and I'm not sure we want to track this, especially given the fact that it probably shouldn't be installed in the first place on a CI environment.
We lack local folder detection for Mac, as I have limited access to this OS for now.

lacostej · 2017-12-26T10:04:18Z

lib/u3d/installation.rb

      end
+      pack << 'Documentation' if specific_dir?("#{root_path}/Editor/Data/Documentation/", check_empty: true)


we could have a method under each PlatformInstallation called

def module_files_pattern(module_name) return "#{root_path}/Editor/Data/Documentation/*" if module_name == "Documentation" return .... end

or something similar

and then do a

['Documentation', 'StandardAssets', 'MonoDevelop'].each do |module_name| patern = module_files_pattern(module_name) pack << module_name if Dir.glob(pattern).count > 0 end

better idea?

I installed 2017.3.0f3 on the first slave with all sub-modules, in case you need to find the paths.

lacostej · 2017-12-26T10:05:22Z

lib/u3d/commands.rb

-              UI.important "Ignoring #{pack} module, it is already installed" if packages.delete(pack)
+            installed_packages = unity.packages
+            packages.each do |pack|
+              if installed_packages.include?(pack) || installed_packages.any? { |installed| !package_aliases[pack].nil? && package_aliases[pack].include?(installed) }


feel free to move this whole logic into a method "package_installed?" or similar.

lacostej · 2017-12-26T10:06:23Z

lib/u3d/commands.rb

+            installed_packages = unity.packages
+            packages.each do |pack|
+              if installed_packages.include?(pack) || installed_packages.any? { |installed| !package_aliases[pack].nil? && package_aliases[pack].include?(installed) }
+                UI.important "Ignoring #{pack} module, it is already installed" if packages.delete(pack)


Does the if need to go away?

packages.delete(pack) UI.important "Ignoring #{pack} module, it is already installed"

niezbop · 2017-12-29T16:33:26Z

Implemented modifications and the Mac part. Example question is still not solved, should we try to detect it as well?

lacostej · 2017-12-29T17:12:31Z

lib/u3d/commands.rb

+        result
+      end
+
+      def package_installed?(package, installed_packages)


would it be better to move this to unity? I don't think command should have too much logic anyway.

E.g. if unity.package_installed? existed, it would be less code here.

lacostej · 2017-12-29T17:12:57Z

lib/u3d/installation.rb

+      when 'MonoDevelop'
+        return "#{root_path}/MonoDevelop.app/"
+      else
+        return ''


fail here instead of silent return

lacostej

Let's also give a good name to this PR. And make it so that it's description explicitly mark the proper issues as closed.

lacostej · 2017-12-29T20:32:32Z

lib/u3d/installation.rb

+      when 'MonoDevelop'
+        return "#{root_path}/MonoDevelop.app/"
+      else
+        UI.user_error! "No pattern is known for #{module_name} on Mac"


maybe crash! instead?

lacostej · 2017-12-29T20:38:35Z

spec/support/installations.rb

@@ -95,5 +95,6 @@ def fake_installation(version, packages: [])
  allow(unity).to receive(:version) { version }
  allow(unity).to receive(:root_path) { 'foo' }
  allow(unity).to receive(:packages) { packages }
+  unity.stub(:package_installed?) { |arg| packages.include?(arg) }


Maybe keep using allow instead of the somewhat deprecated stub?

https://relishapp.com/rspec/rspec-mocks/v/2-14/docs/method-stubs/stub-with-substitute-implementation

lacostej · 2017-12-29T20:40:54Z

lib/u3d/installation.rb

+      PlaybackEngineUtils.list_module_configs(root_path).each do |mpath|
+        pack << PlaybackEngineUtils.module_name(mpath)
+      end
+      %w[Documentation StandardAssets MonoDevelop].each do |module_name|


Let's move that to a module or Installation class constant. Maybe something like SPECIAL_PACKAGES? Better name idea?

lacostej · 2017-12-29T20:44:55Z

lib/u3d/installation.rb

+    end
+
+    def package_installed?(package)
+      return false unless packages


we can replace the two lines with

return true if (packages || []).include?(package)

and maybe change the default packages implementation to return a [] instead of false?

lacostej · 2017-12-29T20:46:56Z

lib/u3d/installation.rb

+      return true if packages.include?(package)
+
+      aliases = Installation.package_aliases[package]
+      return false if aliases[package].nil?


I don't think aliases[package] can get nil. You meant empty right?

In which case, we may just skip the line entirely as it is covered by the next one?

I believe it can when the package is not known ie aliases['NewPackage'] = nil. It makes it so that we don't crash when some package is not known, which would make u3d unusable for versions with unknown packages. So this line is mandatory imo.

lacostej · 2017-12-31T14:30:30Z

lib/u3d/installation.rb

@@ -44,6 +46,37 @@ def self.create(root_path: nil, path: nil)
        WindowsInstallation.new(root_path: root_path, path: path)
      end
    end
+
+    def self.package_aliases


This could be cached as well into a PACKAGE_ALIASES and documented.

lacostej · 2017-12-31T14:56:09Z

We should add some tests to the Installation classes changes at least. I am sure things will break when we have different Unity versions.

niezbop added 7 commits December 24, 2017 17:00

Windows: detect Documentation and Standard Assets

972b860

Rubocop compliance

b16d93f

Add package mapping between available and installed

86827fd

Add alias for AppleTV, Facebook and Vuforia

86ec975

Fix no alias packages

49ca444

Fix package detection

b87064e

Optimize installed package fetching

2d65923

niezbop requested a review from lacostej December 26, 2017 01:53

niezbop self-assigned this Dec 26, 2017

Rubocop compliance

142b2a7

lacostej reviewed Dec 26, 2017

View reviewed changes

niezbop added 4 commits December 29, 2017 16:55

Refactor pattern check for Windows

bbea531

Implement pattern check for Mac

8d843b9

Refactor package skipping when installing packages

8e3454c

Rubocop compliance

d9bd726

lacostej reviewed Dec 29, 2017

View reviewed changes

niezbop added 3 commits December 29, 2017 18:36

Move installed package check to Installation class

66b1b31

Fail when no pattern is found

d3ab768

Stub package installed in Installation

d4671d7

niezbop changed the title ~~[WIP] Correct installed package detection~~ Correct installed package detection Dec 29, 2017

lacostej requested changes Dec 29, 2017

View reviewed changes

niezbop added 3 commits December 31, 2017 15:08

Simplify installed package detection

7d48d67

Use UI.crash! instead of UI.user_error!

c023b46

Use allow instead of stub

7f1f4df

niezbop changed the title ~~Correct installed package detection~~ Match available package name with installed name Dec 31, 2017

Extract not playback engine packages to class constant

b28a9d5

lacostej changed the title ~~Match available package name with installed name~~ u3d/install: do not reinstall already installed packages Dec 31, 2017

lacostej reviewed Dec 31, 2017

View reviewed changes

lacostej approved these changes Dec 31, 2017

View reviewed changes

niezbop added 2 commits January 2, 2018 09:53

Move package aliases to class constant

f0c7ca9

Fix and comment missing package alias

0dba638

niezbop merged commit 96eaeb3 into DragonBox:master Jan 2, 2018

niezbop mentioned this pull request Jan 2, 2018

Incorrect package check downloads and reinstalls it #198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

u3d/install: do not reinstall already installed packages #202

u3d/install: do not reinstall already installed packages #202

niezbop commented Dec 26, 2017 •

edited

Loading

niezbop commented Dec 26, 2017

lacostej Dec 26, 2017

lacostej Dec 26, 2017

lacostej Dec 26, 2017

lacostej Dec 26, 2017

niezbop commented Dec 29, 2017

lacostej Dec 29, 2017

lacostej Dec 29, 2017

lacostej left a comment

lacostej Dec 29, 2017

lacostej Dec 29, 2017

lacostej Dec 29, 2017

lacostej Dec 29, 2017

lacostej Dec 29, 2017

niezbop Dec 31, 2017 •

edited

Loading

lacostej Dec 31, 2017

lacostej commented Dec 31, 2017

		end
		pack << 'Documentation' if specific_dir?("#{root_path}/Editor/Data/Documentation/", check_empty: true)

u3d/install: do not reinstall already installed packages #202

u3d/install: do not reinstall already installed packages #202

Conversation

niezbop commented Dec 26, 2017 • edited Loading

niezbop commented Dec 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niezbop commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lacostej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niezbop Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lacostej commented Dec 31, 2017

niezbop commented Dec 26, 2017 •

edited

Loading

niezbop Dec 31, 2017 •

edited

Loading