Bug 85791 – Remove CYGWIN=tty from environment variable as its no longer supported

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Vivek Galatage]

cygwin throws a warning as:
"tty" option detected in CYGWIN environment variable.
CYGWIN=tty is no longer supported.  Please remove it from your
CYGWIN environment variable and use a terminal emulator like mintty,
xterm, or rxvt.

[Vivek Galatage]

This has been mentioned in the changelog: http://cygwin.com/cygwin-ug-net/ov-new1.7.html


"The CYGWIN environment variable options "envcache", "strip_title", "title", "tty", and "upcaseenv" have been removed."

[Vivek Galatage]

Created attachment 140511 [details]
Patch

[Adam Roben (:aroben)]

What effect will this have for people who haven't yet upgraded to Cygwin 1.7.10?

[Vivek Galatage]

According to http://cygwin.com/cygwin-ug-net/using-cygwinenv.html, this options will: 

(no)tty - If set, Cygwin enabled extra support (i.e., termios) for UNIX-like ttys in the Windows console. This option has been removed because it can be easily replaced by using a terminal like mintty, and it does not work well with some Windows programs.

As on windows, mintty is default terminal program even for older releases of cygwin, I think there should not be any issue.

More info about termios: http://docs.python.org/library/termios.html

[Darin Adler]

Comment on attachment 140511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140511&action=review

> Tools/ChangeLog:8
> +        Cygwin has removed support for the CYGWIN=tty environment variable hence removed it.

No need for this for people using older versions of Cygwin?

[Vivek Galatage]

(In reply to comment #5)
> (From update of attachment 140511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140511&action=review
> 
> > Tools/ChangeLog:8
> > +        Cygwin has removed support for the CYGWIN=tty environment variable hence removed it.
> 
> No need for this for people using older versions of Cygwin?

The usage of mintty is the default option, so this CYGWIN=tty is obsolete a long time ago.

I could dig through a discussion thread at http://www.mail-archive.com/[e-mail]/msg116237.html and found it should be ok to remove this option.

What would be your opinion?

[Adam Roben (:aroben)]

Comment on attachment 140511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140511&action=review

>>> Tools/ChangeLog:8
>>> +        Cygwin has removed support for the CYGWIN=tty environment variable hence removed it.
>> 
>> No need for this for people using older versions of Cygwin?
> 
> The usage of mintty is the default option, so this CYGWIN=tty is obsolete a long time ago.
> 
> I could dig through a discussion thread at http://www.mail-archive.com/[e-mail]/msg116237.html and found it should be ok to remove this option.
> 
> What would be your opinion?

Bug 31228 comment 1 says that CYGWIN=tty is required for emacs to work. If that was ever true, then it seems like this patch will break emacs for people who have an existing Cygwin <1.7.10 install and start working on WebKit for the first time.

[Vivek Galatage]

(In reply to comment #7)
> Bug 31228 comment 1 says that CYGWIN=tty is required for emacs to work. If that was ever true, then it seems like this patch will break emacs for people who have an existing Cygwin <1.7.10 install and start working on WebKit for the first time.

Oh yes, I agree that this would break existing cygwin < 1.7.10 and those who are using programs like emacs with cygwin.

So shall we leave this variable as it is? Or we take a poll on webkit-dev for how many people use windows with cygwin < 1.7.10 and with emacs. So then based on that number make a decision whether to remove this variable.

Your thoughts?

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #7)
> > Bug 31228 comment 1 says that CYGWIN=tty is required for emacs to work. If that was ever true, then it seems like this patch will break emacs for people who have an existing Cygwin <1.7.10 install and start working on WebKit for the first time.
> 
> Oh yes, I agree that this would break existing cygwin < 1.7.10 and those who are using programs like emacs with cygwin.
> 
> So shall we leave this variable as it is? Or we take a poll on webkit-dev for how many people use windows with cygwin < 1.7.10 and with emacs. So then based on that number make a decision whether to remove this variable.
> 
> Your thoughts?

This will only affect people who are doing doing WebKit development for the first time. If you're on a brand new computer, you'll have Cygwin >= 1.7.10 so this won't be a problem. It will only be a problem if you have an existing Cygwin install and start working on WebKit for the first time. I doubt there are too many people in that situation.

But maybe we can make it work for everyone. Can't we detect the Cygwin version using `uname`?

[Vivek Galatage]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Bug 31228 comment 1 says that CYGWIN=tty is required for emacs to work. If that was ever true, then it seems like this patch will break emacs for people who have an existing Cygwin <1.7.10 install and start working on WebKit for the first time.
> > 
> > Oh yes, I agree that this would break existing cygwin < 1.7.10 and those who are using programs like emacs with cygwin.
> > 
> > So shall we leave this variable as it is? Or we take a poll on webkit-dev for how many people use windows with cygwin < 1.7.10 and with emacs. So then based on that number make a decision whether to remove this variable.
> > 
> > Your thoughts?
> 
> This will only affect people who are doing doing WebKit development for the first time. If you're on a brand new computer, you'll have Cygwin >= 1.7.10 so this won't be a problem. It will only be a problem if you have an existing Cygwin install and start working on WebKit for the first time. I doubt there are too many people in that situation.
> 
> But maybe we can make it work for everyone. Can't we detect the Cygwin version using `uname`?

Yes `uname -r` would give which version of cygwin is being used. Something like:

$ uname -r
1.7.11(0.260/5/3)

So in the python script we can detect this and based on the detected version we may or may not set the env. variable. This way it will work for older versions of cygwin as well.

[Vivek Galatage]

Created attachment 140997 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 140997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140997&action=review

This is looking close!

> Tools/ChangeLog:6
> +        Added check to support older versions of Cygwin 1.7.9 or lesser

This doesn't really describe the entire change; it only describes the differences from the last version of your patch.

> Tools/Scripts/webkitdirs.pm:1491
> +        # FIXME: The setting CYGWIN=tty is supported only till cygwin version 1.7.9

Why is this a FIXME? I don't think there's anything we need to "fix" here.

> Tools/Scripts/webkitdirs.pm:1495
> +        my $currentCygwinVersion = version->parse(`uname -r`);
> +        my $minCygwinVersion = version->parse("1.7.10");
> +        if ($currentCygwinVersion < $minCygwinVersion) {

I didn't know about the version module! Seems less mysterious than using "v1.7.10" strings.

I'd rename $minCygwinVersion to $firstCygwinVersionWithoutTTYVariable for clarity.

[Vivek Galatage]

(In reply to comment #12)

Thank you Adam for your review comments.

> (From update of attachment 140997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140997&action=review
> 
> This is looking close!
> 
> > Tools/ChangeLog:6
> > +        Added check to support older versions of Cygwin 1.7.9 or lesser
> 
> This doesn't really describe the entire change; it only describes the differences from the last version of your patch.
> 

I will update the description.

> > Tools/Scripts/webkitdirs.pm:1491
> > +        # FIXME: The setting CYGWIN=tty is supported only till cygwin version 1.7.9
> 
> Why is this a FIXME? I don't think there's anything we need to "fix" here.
> 

I thought as we move further, this check wouldn't be necessary so someday in future we can remove this altogether hence added FIXME

> > Tools/Scripts/webkitdirs.pm:1495
> > +        my $currentCygwinVersion = version->parse(`uname -r`);
> > +        my $minCygwinVersion = version->parse("1.7.10");
> > +        if ($currentCygwinVersion < $minCygwinVersion) {
> 
> I didn't know about the version module! Seems less mysterious than using "v1.7.10" strings.
>
> I'd rename $minCygwinVersion to $firstCygwinVersionWithoutTTYVariable for clarity.

I will update the variable.

[Adam Roben (:aroben)]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 140997 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140997&action=review
> > > Tools/Scripts/webkitdirs.pm:1491
> > > +        # FIXME: The setting CYGWIN=tty is supported only till cygwin version 1.7.9
> > 
> > Why is this a FIXME? I don't think there's anything we need to "fix" here.
> > 
> 
> I thought as we move further, this check wouldn't be necessary so someday in future we can remove this altogether hence added FIXME

In that case, I'd phrase it like so:

FIXME: We should remove this code entirely once we stop supporting Cygwin 1.7.9.

That way it's actually saying what we should fix.

[Vivek Galatage]

Created attachment 141003 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 141003 [details]
Patch

Looks great! Thanks for pushing this to its current state.

[Vivek Galatage]

(In reply to comment #16)
> (From update of attachment 141003 [details])
> Looks great! Thanks for pushing this to its current state.

Thank you too for your quick reviews, appreciated.

[WebKit Review Bot]

Comment on attachment 141003 [details]
Patch

Clearing flags on attachment: 141003

Committed r116559: <http://trac.webkit.org/changeset/116559>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 141003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141003&action=review

> Tools/Scripts/webkitdirs.pm:32
> +use version;

This line seems to have broken my use of build-webkit:

Can't locate version.pm in @INC (@INC contains: /Users/abarth/svn/webkit/Tools/Scripts /opt/local/lib/perl5/site_perl/5.8.9/darwin-2level /opt/local/lib/perl5/site_perl/5.8.9 /opt/local/lib/perl5/site_perl /opt/local/lib/perl5/vendor_perl/5.8.9/darwin-2level /opt/local/lib/perl5/vendor_perl/5.8.9 /opt/local/lib/perl5/vendor_perl /opt/local/lib/perl5/5.8.9/darwin-2level /opt/local/lib/perl5/5.8.9 .) at /Users/abarth/svn/webkit/Tools/Scripts/webkitdirs.pm line 32.

Any ideas?

[Adam Barth]

Note: I'm using a Lion.

[Adam Barth]

I think this is because I'm using a MacPorts version of Perl.  Probably means its a problem on my end.  Thanks!