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

Load Windows color support properly #356

Merged
merged 3 commits into from Nov 26, 2013
Merged

Load Windows color support properly #356

merged 3 commits into from Nov 26, 2013

Conversation

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Nov 25, 2013

This is a potential fix for #352. It makes sure 'Win32/Console/ANSI' is required as soon as possible, i.e. when the cli.rb file is loaded. (This should have been here all along, because it does not really make sense to load color support when setting up cleaning streams.)

Minor changes:

  • Created Nanoc.on_windows? method that is referenced everywhere else, so that the RUBY_PLATFORM check only happens in a single location
  • Added many more Windows-ish platforms to the RUBY_PLATFORM check (not strictly necessary, but better to do it right, I figured)

@raphinesse Can you verify whether this patch works on Windows?

@@ -13,6 +13,11 @@ def self.version_information
res
end

# @return [Boolean] True if the current platform is Windows,
def self.on_windows?
!!(RUBY_PLATFORM =~ /windows|cygwin|bccwin|cygwin|djgpp|mingw|mswin|wince/i)
Copy link
Member Author

@ddfreyne ddfreyne Nov 25, 2013

Choose a reason for hiding this comment

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

I’ve also seen variations of this regex that include emx and vista. Maybe those should be added here too?

Loading

Copy link
Contributor

@raphinesse raphinesse Nov 25, 2013

Choose a reason for hiding this comment

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

I really can't say which variations are actually out there but you have cygwin twice.

However, I thought about a potentially more reliable method. Every Windows I know of has an environment variable WINDIR which denotes the Windows installation directory. So !!ENV['WINDIR'] should do the trick regardless of used compiler and Windows version. List of Environment Variables in Windows XP, Vista and 7. Can't confirm for Windows 8 but I'm pretty sure that it's still there since a lot of stuff depends on this AFAIK. Confirmation of availability of WINDIR from Vista to 8.1

Loading

Copy link
Contributor

@raphinesse raphinesse Nov 25, 2013

Choose a reason for hiding this comment

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

There's also ENV['OS'] which should contain Windows_NT for all recent Windows versions, but I couldn't find documentation for that.

Loading

Copy link
Member Author

@ddfreyne ddfreyne Nov 26, 2013

Choose a reason for hiding this comment

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

The problem with checking environment variables is that they could also be set in non-Windows OSes, for a variety of reasons. I’d rather stick to RUBY_PLATFORM.

Loading

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Nov 25, 2013

Looks good, I have colors! 😀

Two small changes to Nanoc::TestHelpers setup and teardown:

# Go quiet
unless ENV['QUIET'] == 'false'
  @orig_stdout = $stdout
  @orig_stderr = $stderr

  $stdout = StringIO.new
  $stderr = StringIO.new
end
# Go unquiet
unless ENV['QUIET'] == 'false'
  $stdout = @orig_stdout
  $stderr = @orig_stderr
end

I also get following warning when running Nanoc::Extra::Deployers::FogTest#test_run:

←[33m[fog][WARNING] Unable to load the 'unf' gem. Your AWS strings may not be properly encoded.←[0m

But that is because their Logger seems to use STDOUT directly, otherwise the output wouldn't appear at all.

Loading

@ddfreyne
Copy link
Member Author

@ddfreyne ddfreyne commented Nov 26, 2013

@raphinesse Added your #setup/#teardown fix and removed the double cygwin in the RUBY_PLATFORM regex. Do I get a 👍 from you now? ;)

Loading

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Nov 26, 2013

Looks good, yes 👍 Really looking forward to using nanoc with color support 😀

Loading

ddfreyne added a commit that referenced this issue Nov 26, 2013
Load Windows color support properly
@ddfreyne ddfreyne merged commit b6a24a3 into release-3.6.x Nov 26, 2013
@ddfreyne ddfreyne deleted the bug/windows-colors branch Nov 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants