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/dependencies: add command to install Linux dependencies #25

Merged
merged 8 commits into from
Sep 5, 2017

Conversation

niezbop
Copy link
Member

@niezbop niezbop commented Aug 5, 2017

Before installing Unity on Linux, the installer will now install its required dependencies.

@lacostej
Copy link
Member

lacostej commented Aug 5, 2017

A few things make me uncertain about how the current proposal

  1. we force install of the dependencies everytime we try to install. Dependencies are (to my knowledge) for running the editor. Do we need them for installation?
  2. we force install of the dependencies everytime we try to install and regardless if the dependencies are there already
  3. we fail hard if we don't recognize the platform (deb/yum-rpm). This prevents installing unity completely on those systems
  4. the check dev vs rpm doesn't allow us to ensure you are on the right platform (if for example you can install dpkg on a rpm based platform).
  5. Fedora migrated to dnf. Not sure how that works there.

I would like this to be tested heavily on various systems. Maybe implemented as a separate command (u3d install-editor-deps [--type deb/rpm] ?

@niezbop
Copy link
Member Author

niezbop commented Aug 5, 2017

I agree it is a peculiar feature to have. Missing libraries did indeed have no effect on whether the installer completed its job.

  1. Short answer no, but what good is it to install something that won't work?

  2. That's why I thought that validation for each library would be better, as a user could specify to skip some of them, knowing they're already present, or not willing to update them.
    Alternatively, we could either keep track of what has been installed or try to fetch installed libraries before trying to install them.

  3. return and UI.error then?

  4. I'm not sure there is a really better way, it seems to be a well known check. The usecases of people having installed dpkg on a rpm-based os (which has no point as .deb files are Debian specific) is too small (in my opinion at least) to have an impact on this. Then again, there may be a far better check that I'm not aware of! Nothing is written in stone anyways, so my current check could be swapped easily.

  5. I'll try to find a solution for that

This could be a good idea indeed. Especially if we consider the added dependencies needed to build to WebGL or to Tizen/Android, which could be specified in such a command as well.

@lacostej
Copy link
Member

lacostej commented Aug 6, 2017

Notes for self: detect if something is to be installed on Debian derivative:

$ sudo apt-get install -s maven | grep "^Inst"  | wc -l
84
$ sudo apt-get install -s libxi6 | grep "^Inst"  | wc -l
0

Is there a better way?

@lacostej
Copy link
Member

lacostej commented Aug 8, 2017

Short answer no, but what good is it to install something that won't work?

I am not sure I want the question: "do you want to install?" everytime.
An alternative would be to check if there are already some linux versions installed. If not, we could ask. But I don't like to mix the other version presents detection code down the installer. I would prefer an option to the method being passed down (auto_install_deps: true/false). WDYT?

That's why I thought that validation for each library would be better, as a user could specify to
skip some of them, knowing they're already present, or not willing to update them.
Alternatively, we could either keep track of what has been installed or try to fetch installed
libraries before trying to install them.

I prefer the current alternative where I don't want to install all of them. The package manager knows if the packages are there or not. We are not checking version number either. So it's a one shot attempt at getting everything in place.

return and UI.error then?

If it's explicit installation by the user, fail. If it's automatic try installing them, error and return.

I'm not sure there is a really better way, it seems to be a well known check. The usecases of
people having installed dpkg on a rpm-based os (which has no point as .deb files are Debian
specific) is too small (in my opinion at least) to have an impact on this. Then again, there may be
a far better check that I'm not aware of! Nothing is written in stone anyways, so my current check
could be swapped easily.

I agree it's a small case. I would prefer maybe something like

if not packaging system specified
try auto-detecting
if can't
....

I'll try to find a solution for that

OK

I still think we could have a separate action for installing the deps from the CLI to cover the cases.

@lacostej
Copy link
Member

lacostej commented Sep 2, 2017

I propose we merge this in the following way.

  • we add the missing libpq5
  • we add an API so that we can call programmatically something like
    U3d::LinuxDependencies.install(type: :rpm)
  • We don't integrate the call at install time
  • we optionally provide a command. But I can live without this right now.

My objective is to be able to test this on master easily. WDYT?

@niezbop
Copy link
Member Author

niezbop commented Sep 4, 2017

Seems really good to me! I'll work on doing that.

@niezbop niezbop force-pushed the linux_dependencies branch 2 times, most recently from 7cb70ea to ba4b9f3 Compare September 4, 2017 19:20
Add all Unity dependencies and an API to install them. No command is
available right now to install them.
class LinuxDependencies
DEPENDENCIES = [
'gconf-service',
'lib32gcc1',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the dependencies marked like that will be good enough.

E.g. The doc states things like

debconf (>= 0.5) | debconf-2.0

@@ -129,6 +129,15 @@ def install(args: [], options: {})
Installer.install_modules(files, definition.version, installation_path: options[:installation_path])
end

def install_linux_dependencies(options: {})
Copy link
Member

Choose a reason for hiding this comment

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

install_dependencies ?

@@ -148,6 +148,15 @@ def run
end
end

command :dependencies do |c|
c.syntax = 'u3d dependencies [-p | --package_manager <package manager>]'
c.description = 'Installs Unity dependencies. [Linux only]'
Copy link
Member

Choose a reason for hiding this comment

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

summary + description. We can be more verbose on the description part. In particular

  • detail how it works for detecting the package_manager, etc.
  • link to Linux forum post
  • explain how we deal with things like debconf (>= 0.5) | debconf-2.0
  • the fact we only implement the default deps, and not the ones for platforms (WebGL, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

command :dependencies do |c|
c.syntax = 'u3d dependencies [-p | --package_manager <package manager>]'
c.description = 'Installs Unity dependencies. [Linux only]'
c.option '-p', '--package_manager STRING', String, 'Specify which package manager to use (aptitude, rpm...)'
Copy link
Member

Choose a reason for hiding this comment

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

option is unused right now, correct? If so remove it.

Copy link
Member Author

@niezbop niezbop Sep 4, 2017

Choose a reason for hiding this comment

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

Proposed implementation, currently in this PR:

def self.install(prefix: nil)
      unless prefix
        if `which dpkg` != ''
          prefix = 'apt-get -y install'
        elsif `which rpm` != ''
          prefix = 'yum -y install'
        else
          raise 'Cannot install dependencies on your Linux distribution'
        end
      end
      if UI.interactive?
        return unless UI.confirm "Install dependencies? (#{DEPENDENCIES.length} dependency(ies) to install)"
      end
      U3dCore::CommandExecutor.execute(command: "#{prefix} #{DEPENDENCIES.join(' ')}", admin: true)
end

c.syntax = 'u3d dependencies [-p | --package_manager <package manager>]'
c.summary = 'Installs Unity dependencies. [Linux only]'
c.description = %(
#{c.summary}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to remove the front spaces here? Not sure how %( behaves.

c.description = %(
#{c.summary}

Regarding the package manager: if none if specified, u3d will try to assume it by seeing which one.s is.are installed.
Copy link
Member

Choose a reason for hiding this comment

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

various typos.

Regarding the dependencies themselves: only dependencies for the editor are installed. WebGL, Android and Tizen require others that you will have to install manually.
More on that: https://forum.unity3d.com/threads/unity-on-linux-release-notes-and-known-issues.350256/
)
c.option '-p', '--package_manager STRING', String, 'Specify which package manager to use (apt-get, yum...)'
Copy link
Member

Choose a reason for hiding this comment

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

do you really want to use it with -p yum ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a great option, but it was the least bad I could think of. Any suggestion?

Regarding the package manager: if none is specified, u3d will try to assume it by testing which one.s is.are installed.
Only apt-get and yum are tested right now.

If several package manager are installed on your machine simultaneously, we recommend that you specify the one you want to use.
Copy link
Member

Choose a reason for hiding this comment

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

typo: managers


If several package manager are installed on your machine simultaneously, we recommend that you specify the one you want to use.

Regarding the dependencies themselves: only dependencies for the editor are installed. WebGL, Android and Tizen require others that you will have to install manually.
Copy link
Member

Choose a reason for hiding this comment

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

will that work if user doesn't specify -p "apt-get -y" ?

I.e. do we manage input properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not working properly right now, I'm correcting that

Regarding the dependencies themselves: only dependencies for the editor are installed. WebGL, Android and Tizen require others that you will have to install manually.
More on that: https://forum.unity3d.com/threads/unity-on-linux-release-notes-and-known-issues.350256/
)
c.option '-p', '--package_manager STRING', String, 'Specify which package manager to use (apt-get, yum...)'
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -148,6 +148,23 @@ def run
end
end

command :dependencies do |c|
c.syntax = 'u3d dependencies [-p | --package_manager <package manager>]'
Copy link
Member

Choose a reason for hiding this comment

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

update

@niezbop niezbop changed the title Add Linux dependencies u3d/dependencies: Add command to install Linux dependencies Sep 5, 2017
@lacostej lacostej changed the title u3d/dependencies: Add command to install Linux dependencies u3d/dependencies: add command to install Linux dependencies Sep 5, 2017
@niezbop niezbop merged commit 3d2eb75 into DragonBox:master Sep 5, 2017
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