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

Fix #5233: Increase our ability to detect Windows. #5235

Merged
merged 1 commit into from Sep 7, 2016

Conversation

Projects
None yet
6 participants
@envygeeks
Contributor

envygeeks commented Aug 12, 2016

This increases our ability to detect Windows, and to detect Windows+Bash. It also adds a message to Windows for users who try to "--watch", also noting to to them to check out the Windows ticket so eventually somebody pings us if this issue is fixed. /cc @TAGraves

/cc @jekyll/windows @jekyll/core @parkr @benbalter @jekyll/build

@envygeeks envygeeks added this to the 3.2.2 milestone Aug 12, 2016

@envygeeks envygeeks added bug labels Aug 12, 2016

Show outdated Hide outdated lib/jekyll/utils/platforms.rb
def linux?
RbConfig::CONFIG["host_host"] =~ %r!linux! && \
!proc_version =~ %r!microsoft!i

This comment has been minimized.

@parkr

parkr Aug 12, 2016

Member

I might be remembering Lua, but is this !~?

@parkr

parkr Aug 12, 2016

Member

I might be remembering Lua, but is this !~?

This comment has been minimized.

@envygeeks

envygeeks Aug 12, 2016

Contributor

You're right, I was in a hurry and doing it on my tablet, I'll ship a correction.

@envygeeks

envygeeks Aug 12, 2016

Contributor

You're right, I was in a hurry and doing it on my tablet, I'll ship a correction.

This comment has been minimized.

@envygeeks

envygeeks Aug 12, 2016

Contributor

Fixed.

@envygeeks

envygeeks Aug 12, 2016

Contributor

Fixed.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 12, 2016

Member

Love it!!!!!

Member

parkr commented Aug 12, 2016

Love it!!!!!

Show outdated Hide outdated lib/jekyll/utils/platforms.rb
#
def windows?
!!(RbConfig::CONFIG["host_os"] =~ %r!mswin|mingw|cygwin!i || \

This comment has been minimized.

@XhmikosR

XhmikosR Aug 12, 2016

Contributor

How about x64_mingw? Does this work right there?

@XhmikosR

XhmikosR Aug 12, 2016

Contributor

How about x64_mingw? Does this work right there?

This comment has been minimized.

@ghost

ghost Aug 12, 2016

Looks to me like the regex isn't anchored so it'd match x64_mingw

@ghost

ghost Aug 12, 2016

Looks to me like the regex isn't anchored so it'd match x64_mingw

This comment has been minimized.

@envygeeks

envygeeks Aug 16, 2016

Contributor

@XhmikosR @spudowiar we don't care if it's x64, as @spudowiar pointed out, that's why it's unanchored, we are only interested if we find a specific keyword. There could be dozens of builds.

@envygeeks

envygeeks Aug 16, 2016

Contributor

@XhmikosR @spudowiar we don't care if it's x64, as @spudowiar pointed out, that's why it's unanchored, we are only interested if we find a specific keyword. There could be dozens of builds.

This comment has been minimized.

@XhmikosR

XhmikosR Aug 16, 2016

Contributor

As long as it detects x64_mingw as Windows, that's fine.

@XhmikosR

XhmikosR Aug 16, 2016

Contributor

As long as it detects x64_mingw as Windows, that's fine.

Show outdated Hide outdated lib/jekyll/commands/build.rb
if watch_method.parameters.size == 1
watch_method.call(options)
if Utils::Platforms.windows?
Jekyll.logger.warn "", "--watch unsupported on Windows. "

This comment has been minimized.

@benbalter

benbalter Aug 12, 2016

Contributor

Maybe "The --watch flag is unsupported..." for a bit of polish?

@benbalter

benbalter Aug 12, 2016

Contributor

Maybe "The --watch flag is unsupported..." for a bit of polish?

This comment has been minimized.

@envygeeks

envygeeks Aug 16, 2016

Contributor

Done.

@envygeeks

envygeeks Aug 16, 2016

Contributor

Done.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 16, 2016

Contributor

Updated to add #really_windows? so that @ashmaroli can skip certain tests if it's actually Windows.

Contributor

envygeeks commented Aug 16, 2016

Updated to add #really_windows? so that @ashmaroli can skip certain tests if it's actually Windows.

Show outdated Hide outdated lib/jekyll/utils/platforms.rb
private
def proc_version
@cacahed_proc_version ||= begin

This comment has been minimized.

@ashmaroli

ashmaroli Aug 16, 2016

Member

did you intend it to be @cached_proc_version ?

@ashmaroli

ashmaroli Aug 16, 2016

Member

did you intend it to be @cached_proc_version ?

This comment has been minimized.

@envygeeks

envygeeks Aug 16, 2016

Contributor

Fixed.

@envygeeks

envygeeks Aug 16, 2016

Contributor

Fixed.

Fix #5233: Increase our ability to detect Windows.
This increases our ability to detect Windows, and to detect Windows+Bash.  It also adds a message to Windows for users who try to "--watch", also noting to to them to check out the Windows ticket so eventually somebody pings us if this issue is fixed. /cc @TAGraves
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 25, 2016

Contributor

/cc @jekyll/core

Contributor

envygeeks commented Aug 25, 2016

/cc @jekyll/core

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 25, 2016

Member

LGTM. Any objections from @jekyll/Windows?

Member

parkr commented Aug 25, 2016

LGTM. Any objections from @jekyll/Windows?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 29, 2016

Contributor

LGTM.

Contributor

envygeeks commented Aug 29, 2016

LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 29, 2016

Contributor

From our own Windows users this works for them, so it has a LGTM.

Contributor

envygeeks commented Aug 29, 2016

From our own Windows users this works for them, so it has a LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 7, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Sep 7, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit dddafcc into master Sep 7, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @parkr and @envygeeks.

@jekyllbot jekyllbot deleted the update-windows-detection-to-check-proc-version branch Sep 7, 2016

jekyllbot added a commit that referenced this pull request Sep 7, 2016

@parkr parkr added the windows label Sep 7, 2016

@parkr parkr modified the milestones: 3.2.2, 3.3 Sep 20, 2016

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 5, 2016

Member

@envygeeks, is there a way to detect Windows version or detect if I'm on the default Windows cmd.exe instead of bash shell? I miss autoregeneration now..
It works properly when using Jekyll 3.2.1 in cmd.exe on Windows 7.

Member

ashmaroli commented Oct 5, 2016

@envygeeks, is there a way to detect Windows version or detect if I'm on the default Windows cmd.exe instead of bash shell? I miss autoregeneration now..
It works properly when using Jekyll 3.2.1 in cmd.exe on Windows 7.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 5, 2016

Contributor

@ashmaroli we do detect tell the difference between BashOnWindows and Windows itself in cmd.exe/PowerShell, the problem is probably in my detector which is a bit extreme in it's handling, it excludes the entirety of Windows even if there is the win32 extension that works with our watchers.

Later today after I get done with some stuff I'll ship a pull that adds bash_on_windows? and adjusts https://github.com/jekyll/jekyll/pull/5235/files#diff-47014fa0275ac66de0d2217d362529ffR74 to use that method so that watchers still work on normal Windows in cmd/PowerShell.

Contributor

envygeeks commented Oct 5, 2016

@ashmaroli we do detect tell the difference between BashOnWindows and Windows itself in cmd.exe/PowerShell, the problem is probably in my detector which is a bit extreme in it's handling, it excludes the entirety of Windows even if there is the win32 extension that works with our watchers.

Later today after I get done with some stuff I'll ship a pull that adds bash_on_windows? and adjusts https://github.com/jekyll/jekyll/pull/5235/files#diff-47014fa0275ac66de0d2217d362529ffR74 to use that method so that watchers still work on normal Windows in cmd/PowerShell.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 5, 2016

Member

Awesome!! Thanks 😃

Member

ashmaroli commented Oct 5, 2016

Awesome!! Thanks 😃

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 5, 2016

Member

@envygeeks Planning to release 3.3 tomorrow so if you want that change in 3.3, we'll need the pull today. If not, then we can release later.

Member

parkr commented Oct 5, 2016

@envygeeks Planning to release 3.3 tomorrow so if you want that change in 3.3, we'll need the pull today. If not, then we can release later.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 5, 2016

Contributor

I'll push it sometime this evening after I get done working.

Contributor

envygeeks commented Oct 5, 2016

I'll push it sometime this evening after I get done working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment