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

MacOS tcl/tk version mismatch #26

Closed
sunshineco opened this issue Jan 14, 2020 · 16 comments
Closed

MacOS tcl/tk version mismatch #26

sunshineco opened this issue Jan 14, 2020 · 16 comments
Labels
bug Something isn't working regression

Comments

@sunshineco
Copy link

As of fa38ab6, git-gui requires Tcl/Tk 8.6, however, stock MacOS High Sierra is stuck on Tcl/Tk 8.5.9. This makes git-gui completely unusable on Mac out of the box since the version check makes anything lower than 8.6 a fatal error.

git-gui/git-gui.sh

Lines 33 to 43 in 0d2116c

if {[catch {package require Tcl 8.6} err]
|| [catch {package require Tk 8.6} err]
} {
catch {wm withdraw .}
tk_messageBox \
-icon error \
-type ok \
-title "git-gui: fatal error" \
-message $err
exit 1
}

No other part of git-gui requires such an advanced version of Tcl/Tk, so it is unfortunate that git-gui, as a whole, should be held hostage to the relatively minor new feature added by fa38ab6.

Therefore, would it be possible for the new feature to be re-worked so that it falls back to some other method of synchronization if 8.6 is not available, or to avoid performing actions asynchronously for versions less than 8.6? (Or, as a last resort, just disable the new feature when the version is less than 8.6 rather than making git-gui, as a whole, fail outright?)

@prati0100
Copy link
Owner

Cc @logiclrd

Ah, I did fear something like this would happen, but I hoped that since Tcl/Tk 8.6 came out in 2012 almost everyone would have access to that version. Well, Apple apparently didn't get the memo. Also, no one pointed out during the reviews that MacOS is still lagging behind.

And while I am certainly frustrated at Apple for shipping with an out-dated version, still the sensible thing would be to first make sure users can actually run git-gui.

As for why we bumped up the version number, it was because Jonathan used TclOO as the object-oriented framework instead of git-gui's home-grown one. And TclOO is incompatible with any version of Tcl before 8.6. I hoped a little modernisation won't hurt and so decided to keep using the package, especially because Linux and Windows do ship with 8.6, which was what I could manage to test on.

Now most of the TclOO stuff should be pretty easy to port to our homegrown OO framework. The only tricky part I can see is the method unknown in the class ChordNote. AFAIK git-gui's OO framework doesn't support something like that. So if we want to keep using something like that, the homegrown framework needs to be updated to support something like "default methods".

I'll see if I can find some time to hack something together.

On a side note: I suspect you hit this problem because you updated to Git v2.25.0. Am I correct? I'll almost certainly have the fix out by Git v2.26.0 but I'm not sure if the Git maintainer would be willing to release a v2.25.1 just for the git-gui fix. So in the meantime, how about you try removing the check and see if git-gui works on 8.5?

@prati0100 prati0100 added bug Something isn't working regression labels Jan 15, 2020
@sunshineco
Copy link
Author

sunshineco commented Jan 15, 2020

I hoped that since Tcl/Tk 8.6 came out in 2012 almost everyone would have access to that version. Well, Apple apparently didn't get the memo.

That's not at all unusual for Apple. Out-of-the-box GNU make on Mac OS is 3.81 from 2006(!) even though the most recent release from GNU is 4.2.1 from 2016.

Also, no one pointed out during the reviews that MacOS is still lagging behind. I suspect you hit this problem because you updated to Git v2.25.0. Am I correct?

No. I track Junio's next branch pretty faithfully, often updating from that branch daily, though lately perhaps every week or two. It looks like Junio merged these changes after v2.25.0-rc1, which is quite late in the Git release cycle. Perhaps sending him a pull request more frequently (or earlier in the Git release cycle) would help give git-gui changes wider exposure before they end up in a proper release.

I'll almost certainly have the fix out by Git v2.26.0 but I'm not sure if the Git maintainer would be willing to release a v2.25.1 just for the git-gui fix.

It's perhaps something to bring up on the mailing list (at least once the fix is available). Knowing Junio, he's unlikely to rush out a new release even to fix a critical (to Mac OS users) issue like this, but he may include it in a v2.25.1 release (along with other important non-git-gui changes) if he receives the fix earlier rather than later (assuming a v2.25.1 release is warranted some time down the road).

So in the meantime, how about you try removing the check and see if git-gui works on 8.5?

I dropped the version check back to 8.4 locally before opening this issue (though forgot to say so) without seeing any ill-effects to any of the git-gui features I use.

@prati0100
Copy link
Owner

Perhaps sending him a pull request more frequently (or earlier in the Git release cycle) would help give git-gui changes wider exposure before they end up in a proper release.

I consider pushing to my 'master' as a sort of trial phase where people interested in git-gui can test out things, and if no one reports a problem then I ask Junio to pull in the changes. But I suppose not many people watch my repo, so I will start asking Junio to pull in changes more frequently and see how things turn out.

but he may include it in a v2.25.1 release (along with other important non-git-gui changes) if he receives the fix earlier rather than later (assuming a v2.25.1 release is warranted some time down the road).

I don't want to delay the fix too long but I don't have a lot of free time on my hands so I'm not sure when I can finish doing the refactor. I'm pretty positive I'll get it done by 2.26 but I can't promise anything before that.

That said, I'll take a stab at the problem this weekend and see how things turn out.

So in the meantime, how about you try removing the check and see if git-gui works on 8.5?

I dropped the version check back to 8.4 locally before opening this issue (though forgot to say so) without seeing any ill-effects to any of the git-gui features I use.

Interesting! I remember seeing TclOO say that it is incompatible with versions of Tcl earlier than v8.6. But if sourcing the file doesn't lead to any side effects, maybe only a subset of TclOO is incompatible and we're not using that subset.

Can you try doing a Ctrl-J on some unstaged files to see if they get deleted properly? If they do, maybe we can get away with just not bumping the version number for now as a temporary fix until the refactor is done.

@logiclrd
Copy link
Contributor

logiclrd commented Jan 16, 2020

I've taken a little bit of a poke at making older Tcl versions support chord.tcl as-is. I found a library called MeTOO that implements a subset of TclOO for use in Tcl 8.4/8.5 (it appears the use of an 8.5-specific feature crept into it, but I believe I found a work-around for that). But, I'm running into a problem that might be intractable -- I don't truly understand it due to my limited experience with Tcl, but with the MeTOO emulator, a statement like set field 42 in e.g. a class constructor does not create a field that can then be read later in a class method with simply $field. Perhaps there is some alternate syntax for this that chord.tcl could be migrated to.

But, as previously suggested, it's probably a simpler alternative to migrate the code I wrote against TclOO to use class.tcl instead. I have only taken the most cursory of glances at it, but it looks like it resolves the problem described in the first paragraph through explicit declaration of fields, which it then dynamically injects into the executing namespace when a method/constructor is called? And, in the absence of an unknown method, rather than teaching class.tcl how to do that, the unknown method of the ChordNote class could simply be given a name like activate, and the call sites updated to just use a regular method call.

@prati0100
Copy link
Owner

it's probably a simpler alternative to migrate the code

Yes, I would strongly prefer using class.tcl over MeTOO. We'd have to go through the effort of porting it over anyway and I'd rather avoid similar nasty surprises.

in the absence of an unknown method, rather than teaching class.tcl how to do that, the unknown method of the ChordNote class could simply be given a name like activate, and the call sites updated to just use a regular method call.

Yes, that would be a nice idea. Since we pass it in an after callback it shouldn't be a huge change.

So, will you volunteer to do the refactor or should I do it? I want to avoid duplicating our efforts.

@logiclrd
Copy link
Contributor

I could probably look at it in a couple of weeks. If you'd rather get on it sooner, then you should take it on. :-)

@sunshineco
Copy link
Author

Can you try doing a Ctrl-J on some unstaged files to see if they get deleted properly? If they do, maybe we can get away with just not bumping the version number for now as a temporary fix until the refactor is done.

The file doesn't get deleted. The action throws a Tcl error:

invalid command name "SimpleChord"

So, yes, TclOO seems to load successfully (or at least silently), but code which relies upon it fails.

As a short-term fix, maybe just disable that functionality if SimpleChord is not defined?

@venkatb4u
Copy link

I badly need this Git Gui tool. I'm now facing this version conflict fatal error. This is blocking me from using it. Please fix asap

@prati0100
Copy link
Owner

@venkatb4u I have been really busy these past few weeks and have not been able to find much time to work on the fix. And I don't expect to find much free time this weekend either. But I will try to have the fix out by the next one. I am almost halfway done but still need to polish more things up.

Unfortunately for now I think the only option for you is to use an older version of git-gui. Or you can see if it is possible to upgrade to Tcl 8.6 on Mac. Check here. My workplace's firewall blocks the site so I can't say for sure if you will find a link for MacOS.

Or you can fix the problem yourself and help everyone else :)

@johannes87
Copy link

homebrew now has a "git-gui" formula that works

@sunshineco
Copy link
Author

I badly need this Git Gui tool. I'm now facing this version conflict fatal error. This is blocking me from using it.

You can get git-gui working locally easily enough by modifying the version check to once again only require 8.4 rather than 8.6. Change these lines near the top of git-gui.tcl:

if {[catch {package require Tcl 8.6} err]
 || [catch {package require Tk  8.6} err]

to instead say:

if {[catch {package require Tcl 8.4} err]
 || [catch {package require Tk  8.4} err]

You may need to search around to find the installed git-gui.tcl. In my case, it's installed at /usr/local/git/share/git-gui/lib/git-gui.tcl.

git-gui works just fine with this local modification; the only thing which won't work is the single new feature which prompted the version bump in the first place, but if you don't use that feature, then you should experience no problem.

@prati0100
Copy link
Owner

prati0100 commented Mar 15, 2020

Fix posted at here. You can also get them from GitHub here.

Please test and let me know if something isn't working. git-gui should open without the version mismatch popup. Please also try reverting some unstaged and untracked files to be sure I didn't miss anything.

@sunshineco
Copy link
Author

Please test and let me know if something isn't working. git-gui should open without the version mismatch popup.

With these patches applied, git-gui correctly launches on Mac OS's old tcl/tk.

Please also try reverting some unstaged and untracked files to be sure I didn't miss anything.

The "revert" function also seems to work correctly on both staged and unstaged files.

One issue I noticed, though, is that if you press the "Do Nothing" button in response to the prompt "Do you really want to revert/delete the file?", the application becomes dysfunctional -- it is no longer possible to interact with the file list. I suspect that this misbehavior is unrelated to the fix you provide here and was probably present in the original implementation provided by fa38ab6 (though I didn't check since I couldn't run fa38ab6 on Mac OS). If so, that problem probably deserves its own issue.

At any rate, this issue (#26) can probably be closed.

@prati0100
Copy link
Owner

One issue I noticed, though, is that if you press the "Do Nothing" button in response to the prompt "Do you really want to revert/delete the file?", the application becomes dysfunctional -- it is no longer possible to interact with the file list. I suspect that this misbehavior is unrelated to the fix you provide here and was probably present in the original implementation provided by fa38ab6 (though I didn't check since I couldn't run fa38ab6 on Mac OS). If so, that problem probably deserves its own issue.

I can't believe I forgot to test that 🤦‍♂️. It is a problem with this series. Fixed in v2.

Thanks for testing. Please let me know if you find anything else.

@sunshineco
Copy link
Author

Thanks for testing. Please let me know if you find anything else.

I did some light testing of v2, and it seems to work as expected, both in the sense that git-gui can now be launched on Mac OS and in the sense that the "revert untracked files by deleting them" feature works (including the "Do Nothing" button). Thanks for addressing the problem. This ticket can probably now be closed.

@prati0100
Copy link
Owner

Fixed in a572802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression
Projects
None yet
Development

No branches or pull requests

5 participants