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.

Sign up for GitHub

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

Jump to bottom

Linux versions fix #6

Merged
merged 2 commits into from
Aug 2, 2017
Merged

Linux versions fix #6

merged 2 commits into from
Aug 2, 2017

Conversation

niezbop
Copy link
Member

@niezbop niezbop commented Jul 30, 2017 •
edited
Loading

The LOG_RULES.md had some typos left, and some inconsistencies that needed tackling.

The installer.rb undergoes two modifications:

  • The Runner class, used to run Unity, has been moved to a separate file to further increase clarity.
  • Installer was asking to sanitize at each package installation when several were installed at once. Minor changes have been made so that the sanitation is offered only once. NOTE: the installation methods are now instance methods and no longer class methods.

Small fix regarding Linux versions, versions above 5.5.0 should now be listed as well.

lacostej
lacostej previously approved these changes Jul 30, 2017
View reviewed changes
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.

Looks fine to me.

@lacostej
Copy link
Member

Does code calling find_projectpath_in_args need change?

lacostej
lacostej reviewed Jul 30, 2017
View reviewed changes
lib/u3d/unity_runner.rb Outdated
# Launches Unity with given arguments
class Runner
def run(installation, args, raw_logs: false)
require 'fileutils'
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected, and commit amended

@niezbop
Copy link
Member Author

niezbop commented Jul 30, 2017

find_projectpath_in_args calls are tied with the runner anyways so it will not create any problem. The only call to this method is in the run command and it is still working as intended!

@niezbop niezbop changed the title Documentation corrections & Installer changes Documentation corrections, Installer changes & Linux versions fix Jul 30, 2017
lacostej
lacostej reviewed Jul 30, 2017
View reviewed changes
lib/u3d/unity_version_number.rb Outdated
@@ -43,7 +43,7 @@ def to_s
class UnityVersionComparator
include Comparable

RELEASE_LETTER_STRENGTH = { a: 1, b: 2, f: 3, p: 4 }.freeze
RELEASE_LETTER_STRENGTH = { a: 1, b: 2, f: 3, p: 4, xa: 1, xb: 2, xf: 3, xp: 4 }.freeze
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a separate fix for the Linux fix, in particular with unit tests for the elements we have tests for. E.g. the UnityVersionComparator, etc.

@lacostej lacostej dismissed their stale review July 30, 2017 22:24

Changes added not related to original fix.

@lacostej
Copy link
Member

lacostej commented Aug 1, 2017

First 3 commits merged. Can you pull rebase this PR so that we can treat the Linux part on its own? Thanks!

@niezbop
[FIX] Available versions on Linux > 5.5.0
7ee7851 
Unity changed the listing for Linux versions above 5.5.0, and they
weren't listed properly.
This fixes this issue and also takes care of the special Linux
version naming ('0.0.0xf0')
@niezbop niezbop changed the title Documentation corrections, Installer changes & Linux versions fix Linux versions fix Aug 1, 2017
@niezbop
Parse linux versions as regular versions
e807eec 
The Linux installer files often came with an 'x' in front of the release
type letter. It is not meant to be permanent and u3d parses it as a
regular version by getting rid of this x whenever necessary.
@niezbop
Copy link
Member Author

niezbop commented Aug 2, 2017

Changed the behaviour of the unity versions lister so linux versions are now parsed as regular ones.

lacostej
lacostej reviewed Aug 2, 2017
View reviewed changes
lib/u3d/unity_versions.rb
if capt && capt[1] && capt[2]
versions[capt[2].gsub(/x/, '')] = capt[1]
else
UI.error("Could not retrieve a fitting file from #{url}")
Copy link
Member

Choose a reason for hiding this comment

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

I'll accept the patch as is. For future improvements, I wonder if we should fail harder here (throw exceptions)

@lacostej lacostej merged commit dd954ca into DragonBox:master Aug 2, 2017
lacostej added a commit to lacostej/u3d that referenced this pull request Aug 3, 2017
@lacostej
Do not try to delete non real logFile files (/dev/stdout support) Dra…
6c5ab9c 
…gonBox#6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lacostej lacostej lacostej left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@niezbop @lacostej