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.
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."
Created attachment 140511 [details] Patch
What effect will this have for people who haven't yet upgraded to Cygwin 1.7.10?
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
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?
(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/cygwin@cygwin.com/msg116237.html and found it should be ok to remove this option. What would be your opinion?
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/cygwin@cygwin.com/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.
(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?
(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`?
(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.
Created attachment 140997 [details] Patch
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.
(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.
(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.
Created attachment 141003 [details] Patch
Comment on attachment 141003 [details] Patch Looks great! Thanks for pushing this to its current state.
(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.
Comment on attachment 141003 [details] Patch Clearing flags on attachment: 141003 Committed r116559: <http://trac.webkit.org/changeset/116559>
All reviewed patches have been landed. Closing bug.
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?
Note: I'm using a Lion.
I think this is because I'm using a MacPorts version of Perl. Probably means its a problem on my end. Thanks!