From: SourceForge.net <no...@so...> - 2012-09-07 12:21:23
|
Bugs item #3562640, was opened at 2012-08-28 23:28 Message generated for change (Comment added) made by dgp You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3562640&group_id=10894 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: 80. Thread Package Group: development: 8.6b2 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Jan Nijtmans (nijtmans) Assigned to: Jan Nijtmans (nijtmans) Summary: problem loading Thread in 8.5, when compiled for 8.6 Initial Comment: (Reported on Tcl because Thread is not the only package suffering from this) In Tcl 8.6, (interp)->errorLine became deprecated. In order to prevent the deprecation warning, extensions should do something like: #if (TCL_MAJOR_VERSION == 8) && (TCL_MINOR_VERSION < 6) #define ERRORLINE(interp) ((interp)->errorLine) #else #define ERRORLINE(interp) (Tcl_GetErrorLine(interp)) #endif The problem with this is that when Thread is compiled using Tcl 8.6 but run on Tcl 8.5, the Tcl_GetErrorLine call will crash because Tcl 8.5 does not have it. ---------------------------------------------------------------------- >Comment By: Don Porter (dgp) Date: 2012-09-07 05:21 Message: Thread 2.7 currently fails the important function of supporting a script that wants to do this: package require Thread 2.7 ... thread::cancel .... I have such a script. It's the thread.test file in the Tcl 8.6 test suite. If it happens to pick up Thread 2.7 that was built against 8.5 headers then the [thread::cancel] command is not present and the script fails. I have seen this happen. Now the script could check for the existence of the command too, but that pretty much negates the value of gathering sets of commands into collections and calling them packages now doesn't it? Revising Thread 2.7 to stop lying and claiming it can provide its command set on top of a Tcl 8.5 foundation is the simple and correct solution. I intend to put that solution in place on the Thread trunk today, in service of another Tcl+pkgs rc snapshot ASAP. ---------------------------------------------------------------------- Comment By: Stuart Cassoff (stwo) Date: 2012-09-06 16:47 Message: Isn't this getting a bit silly? ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-09-06 14:15 Message: > Thus Thread 2.7 ought to be calling > Tcl_InitStubs(... "8.6" ...) (whether or not we > spell it that way or as TCL_VERSION :) ). And Thread 2.6 ought to be calling Tcl_InitStubs(... "8.4" ...), which it, thankfully, does. Thread 2.6 doesn't use any 8.5+ API's but it uses Tcl_GetErrorLine() if compiled against 8.6 headers.... So Thread 2.6 has the same bug..... and it isn't 'violating' using API's outside it's [package require] expectations..... > Well, for Thread 2.7 specifically, it seems > we are in agreement. Either Thread should use a fixed "8.6", or a correct runtime detection of used API's (however difficult that is, I understand that people resent that, it's certainly not advisable!). But Thread already implements the runtime detection, the difficult work has already been done. It would be a waste to throw it away, now that it works. I now backported the bug-3562640 fix to Thread 2.6 as well. So Thread is not using the Tcl_GetErrorLine API any more. It is just using its own macro equivalent to interp->errorLine, which just happens to have the same name as the 8.6 API ;-) ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2012-09-06 13:25 Message: Well, for Thread 2.7 specifically, it seems we are in agreement. Thread 2.7 uses Tcl_CancelEval() a routine found only in Tcl 8.6. Thus Thread 2.7 ought to be calling Tcl_InitStubs(... "8.6" ...) (whether or not we spell it that way or as TCL_VERSION :) ). ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-09-06 13:21 Message: > "There is no way for an extension to eliminate > the deprecation warning and still compile and run > with multiple Tcl versions." I think you'll find that > bug-3562640 achieves that. For 75%. At the moment, Thread 2.7 can compile against 8.5 and 8.6, and it can run with 8.5 and 8.6. One of those 4 possibilities will be lost. > It would not bother > me personally to drop the use of > TCL_DEPRECATED_API. TIP 336 did not call > for that; it was added later. That's would also solve it, but it will - unfortunately - slow down the adoption of Tcl_GetErrorLine() Then we could as well make Tcl_GetErrorLine() a macro equivalent to interp->errorLine, when TCL_NO_DEPRECATED is not set. But ... actually, let's not forget that Zoran is the official maintainer of Thread. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2012-09-06 12:18 Message: The benefit of putting TCL_VERSION in the Tcl_InitStubs() call is that you enlist the compiler's help in determining that you truly are limiting yourself to the API version you claim to need. The (debatable) disadvantage is that you must keep around the header for the earliest version of Tcl you intend your binary to [load] into. This is a somewhat more serious issue when the builder and the author of the extension are not the same person. Training an audience of build-bots that it matters what Tcl header is compiled against is something of an uphill climb. The speed of deprecation here is part of the issue. We may well be over-learning the lesson of interp->result. It would not bother me personally to drop the use of TCL_DEPRECATED_API. TIP 336 did not call for that; it was added later. "There is no way for an extension to eliminate the deprecation warning and still compile and run with multiple Tcl versions." I think you'll find that bug-3562640 achieves that. "... teapot must supply two different thread versions, 2.6 and 2.7, as long as Tcl 8.5 is alive." That's precisely the scenario that Tcl's [package] excels at supporting, so that seems fine to me. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-09-02 13:53 Message: > do you agree that as a general > answer for most extensions, the conventional approach > represented by the patch on the bug-3562640 branch > is what we should advise extension authors to do? The part I don't agree with in the bug-3562640 branch is the usage of @TCL_VERSION@ in pkgIndex.tcl in and the usage of Tcl_InitStubs(interp, TCL_VERSION, 0). If an extension has a minumum required API, no matter if it's "8.1", "8.5" or "8.6". The pkgIndex.tcl file and the Tcl_InitStubs() call should say so. That should NOT depend on the Tcl version the extension is compiled against. Normally, when an API is added in Tcl 8.x, that means the extension has two possibilities. Either use it or not. If the new API is used, It means the extension must increase its minimum required API, and so it cannot be compiled against lower Tcl versions any more. TIP #336 is special in this respect: It does not add an API for new functionality, it provides a different API for functionality that already existed, but needed to be accessed in another way. It used to be a field in Tcl_Interp, now it is a new function. So now we are facing extensions which already used the functionality, but now are forced to use the new API, otherwise they are punished with a deprecation warning. There were more examples of such changes in the past, e.g. TIP's #98, #330, #73 and #139 (The latter two moved API's from private to public stub tables, so theoretically extensions were not allowed to used it, but still it created the same problem for extensions). None of those generated a deprecation warning when the old API was used while compiling against a newer Tcl. TIP #336 is the first TIP doing that, and that's the point here: There is no way for an extension to eliminate the deprecation warning and still compile and run with multiple Tcl versions. So, Thread 2.7 has two choices: Live with the deprecation warning, and function as it does now, or make the conversion and thereby lose support for Tcl 8.5. The latter would mean that teapot must supply two different thread versions, 2.6 and 2.7, as long as Tcl 8.5 is alive. As I said, there is no perfect solution. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2012-08-31 10:37 Message: Setting aside Thread for a moment (since you report the issue to Tcl on the grounds that other extensions will have to have a way to migrate the (interp)->errorLine => Tcl_GetErrorLine() transformation ), do you agree that as a general answer for most extensions, the conventional approach represented by the patch on the bug-3562640 branch is what we should advise extension authors to do? That's the advice I've always intended to record in a wiki page or similar source of migration instruction. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2012-08-31 10:23 Message: As I'm thinking about these matters again, I'm driven further to the opposite conclusion. We are calling this a new minor version because it provides a new command [thread::cancel]. The new command [thread::cancel] cannot function without core support found only in Tcl 8.6. That's why you're going to all the trouble to create runtime switch-ability to disable it when paired with an older core, downgrading it at runtime back to Thread 2.6 capability. Seems to me the clear headed conclusion is that Thread 2.7 ought to [package require Tcl 8.6] and that makes all this strife just go away. Since a Tcl 8.5 interp is never going to get access to anything more than the Thread 2.6 feature set anyway, let them continue to use Thread 2.6. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-30 13:01 Message: The disadvantage of this is that two binary versions will start appearing with the same Thread version number (2.7). Some environments will distribute a Thread 2.7 which is compiled against 8.5. Tcl 8.5 applications will happily be able to make use of it. Then a battery- included Tcl 8.6 is installed with Thread 2.7. Suddenly, Tcl 8.5 applications are not able to use it any more ;-( What I'm trying to say here that there is no perfect solution. But there is a simple solution, even though it's a hack. I think the bug-3562640 branch is a step backwards: Thread already supported a single binary which can handle both 8.5 and 8.6 happily. Thread::cancel support was runtime-switchable. Only one function call was not handled correctly. And for that we are crippling Thread? What a pity. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2012-08-30 11:13 Message: I've opened a branch bug-3562640 that provides the patch that takes the conventional approach on migration matters such as this. I greatly prefer that over what's on the Thread trunk at the moment. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 07:29 Message: Fixed in trunk (Thread), following dkf's suggestion. If someone has a better solution, please re-open! > there's no guarantee that we'll maintain the field *after* 8.6 Yes there is, because we promise upwards binary compatibility. So we need to keep this field for all 8.x releases. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-08-29 05:34 Message: If it's doing (effectively) [package require Tcl 8.5], it shouldn't use Tcl_GetErrorLine as that's introduced in 8.6. That means that there'll be a deprecation notice when building against 8.6 (there's no guarantee that we'll maintain the field *after* 8.6, and certainly not in 9.0) but that's not a big disaster. >> Hacking the API function into 8.5's private space isn't a good solution >> because it still won't be in the existing releases of 8.5 > Agreed, but its better than doing nothing. It's worse, because you'll have people complaining that it doesn't work in 8.5 (8.5.12 really). Look the rule is ever so simple. If you use an 8.6 API, depend on 8.6. Trying to build code that works without recompilation and selects between API versions at runtime is so thoroughly wrongheaded I'm almost speechless. The right way is to do a new version of Thread which depends on 8.6 and which uses the correct API without nasty conditional code. That code will not work in 8.5 or before, but will be fine in 8.6. Or alternatively build it for the two "Tcl platforms" and select which implementation to load in the pkgIndex.tcl (assuming that the two implement the same version of Thread's own API). ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 04:38 Message: > it really needs to do the correct [package requires] as well. That's why it has a: if {[package vsatisfies [package provide Tcl] 8.5] && [::tcl::pkgconfig get threaded]} { because that's the miminum supported version, even if compiled against 8.6 headers. > Hacking the API function into 8.5's private space isn't a good solution > because it still won't be in the existing releases of 8.5 Agreed, but its better than doing nothing. > However, it generates a warning when building against 8.6 to > remind the developer that the field will go away in the future Well, if the developer used: #if (TCL_MAJOR_VERSION == 8) && (TCL_MINOR_VERSION < 6) #define ERRORLINE(interp) ((interp)->errorLine) #else #define ERRORLINE(interp) (Tcl_GetErrorLine(interp)) #endif Then why does it still need to being reminded? He already did what he should do. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-08-29 03:58 Message: BTW, if the thread code is doing something like the construction in the initial message, it really needs to do the correct [package requires] as well. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-08-29 03:55 Message: If an 8.6-specific API is used (such as Tcl_GetErrorLine) then it requires that you have done a [package require Tcl 8.6] (or C equivalent) beforehand. If only the API functions used are 8.5 ones, there's a sporting chance that build with 8.6 and load into 8.5 could work, but it is a Formally Unsupported configuration. Hacking the API function into 8.5's private space isn't a good solution because it still won't be in the existing releases of 8.5. (You can't control what exact versions people use, you just can't. I wish we could get people off 8.0 for example...) For the errorline code (and equivalently the interpreter result) there's a migration strategy: define the correct symbol when building. However, it generates a warning when building against 8.6 to remind the developer that the field will go away in the future. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 03:34 Message: > And building against version 8.X and loading into 8.Y (where Y<X) has never > ever been a supported scenario. So, you are trying to say that Thread made a mistake, supporting to be compiled using 8.6 and runnung under 8.5? I don't think so! Tcl/Tk not supporting that is another story than Thread supporting that. There's code in Thread, related TIP #285 support (thread cancel). The only way to compile Thread with TIP #285 support is compiling it against 8.6 headers. If Thread is running on Tcl 8.5, a runtime-check makes sure that the TIP #285 support is disabled, so a single extension can handle both Tcl 8.5 and 8.6. I think that's well done, even though Tcl doesn't support that officially. Tk 8.6 should never be loadable in Tcl 8.5, I agree with that. But tcldde.dll and tclreg.dll work fine in Tcl 8.5, even when they are compiled against the 8.6 headers. So, we should provide something for Thread (and other extensions) one way or another. Your solution results in deprecation warnings when compiled against 8.6, which cannot be switched off. What I want is provide a way for extensions to upgrade their use of interp->errorLine, without being haunted with deprecation warnings and without causing crashes in difficult-to-debug situation. I gave two alternatives doing that. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-08-29 03:06 Message: Doesn't change a thing. The migration path is to first define USE_INTERP_ERRORLINE, which lets the code compile and work (but with a compile-time warning when building against 8.6). Once they tire of the warning, they can switch to the 8.6-specific API, but that *requires* that they also update what version of the API they depend on. And building against version 8.X and loading into 8.Y (where Y<X) has never ever been a supported scenario. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 01:56 Message: Alternative fix (for Tcl 8.5) committed to branch "bug-3562640-alt". Either "bug-3562640" should be committed to 8.6, or "bug-3562640-alt" to 8.5, or Thread should be compiled with -DUSE_INTERP_ERRORLINE. Otherwise people who did nothing wrong will be confronted with unexplainable crashes, when they have Tcl 8.5 and 8.6 (with thread) together in the same environment. But I would hate to punish Thread for making the conversion to Tcl_GetErrorLine(). Other extensions doing that should be rewarded, not punished, and people using those extensions (who didn't do anything wrong) should not be confronted with unexplainable crashes. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 01:31 Message: Or -- in other words: I don't want to punish people which convert their extension in using Tcl_GetErrorLine(), as thread did. People should be stimulated to do that! So if there is a way to prevent a crash, when someone installs Tcl 8.6 (including a thread compiled against 8.6) in an enviroment where Tcl 8.5 is installed already. And then someone does a "package require thread" from a 8.5-related Tcl application. Thats the tricky situation we are talking about, and I would like to make that work, without extension writers holding back to make the Tcl_GetErrorLine() conversion. One alternative solution (If I cannot convince you) is fill the Tcl 8.5 stub table with an internal Tcl_GetErrorLine. Then - at least starting with Tcl 8.5.13 - the described scenario will start to work. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-29 01:21 Message: > I think that's totally and wholly wrong Somehow, I expected that reaction ;-) ... However, it's just what we want to support. > has *never* been supported, and is not expected to work at all Yes, it works, and everything in Thread is in place to support it except the use of Tcl_GetErrorLine(). A declaration warning on the use of "unused5" is not necessary: Anyone using it is playing dangerous anyway. The alternative is to let thread use -DUSE_INTERP_ERRORLINE, and just continue to use (interp)->errorline. But then the conversion must wait until Tcl 8.5 died. This way we can use my hack as a transition in Tcl 8.6, and remove it in Tcl 8.7: Tcl 8.5 will be dead (hopefully) when 8.7 comes out. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2012-08-29 01:09 Message: I think that's totally and wholly wrong. It's based on a wrong way of working and is wrongheaded in what it does. Wrong, wrong, wrong, wrong, wrong. Compiling against 8.6 and loading into 8.5 is wholly unsupported, has *never* been supported, and is not expected to work at all. That it crashes makes me happy because it prevents people from doing something wrong. Avoiding the deprecation warning by doing weird hacks is wrong; it's there precisely to warn people that their code is doing something that will cease to work. Code should either target 8.5 (defining USE_INTERP_ERRORLINE in case they're actually building against 8.6) or should use Tcl_GetErrorLine and explicitly declare a dependency on the 8.6 API in its [package require]. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2012-08-28 23:31 Message: Proposed solution committed in branch bug-3562640. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3562640&group_id=10894 |