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.

Sign up for GitHub

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

Jump to bottom

Upgrade TclX to Tcl9 #12

Closed
wants to merge 19 commits into from
Closed

Upgrade TclX to Tcl9 #12

wants to merge 19 commits into from

Conversation

ramikhaldi
Copy link

@ramikhaldi ramikhaldi commented May 10, 2020 •
edited
Loading

I have migrated some deprecated APIs and performed some adaptations in order to get the Tclx working with Tcl9 (There are some behavior changes in Tcl9).

The Tcl version used on Travis Ci is 8.6. The good news is that, the changes made for Tcl9 compatibility are also backward compatible with Tcl8.6.

bovine
bovine reviewed May 12, 2020
View reviewed changes
tests/filescan.test Outdated Show resolved Hide resolved
resuna
resuna reviewed May 12, 2020
View reviewed changes
Copy link
Member

@resuna resuna left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have more details on the test errors.

[Edit: I submitted comments this morning but apparently they didn't show up until I made a top level comment]

tests/filescan.test Outdated Show resolved Hide resolved
tests/filescan.test Outdated Show resolved Hide resolved
generic/tclExtend.h Outdated Show resolved Hide resolved
library/globrecur.tcl Outdated Show resolved Hide resolved
@resuna
Copy link
Member

resuna commented May 13, 2020

Can you make this relative to flightaware:tclx/TCL9 so we can merge it to a new branch?

@ramikhaldi ramikhaldi changed the base branch from master to TCL9 May 13, 2020 17:57
@ramikhaldi
Copy link
Author

What's the rationale for this change?

Fixing the convlib incompatibilities ...

@ramikhaldi ramikhaldi mentioned this pull request May 14, 2020
Open
@ramikhaldi
Copy link
Author

ramikhaldi commented May 16, 2020 •
edited
Loading

All required changes have been applied.The travis ci still uses the Tcl8.6 as interpreter. I think, it makes sense to test the migrated library against the newest Tcl9 after merging.

ramikhaldi
ramikhaldi commented May 16, 2020
View reviewed changes
generic/tclXsignal.c Outdated Show resolved Hide resolved
@resuna
Copy link
Member

resuna commented May 29, 2020

The decimal file modes in the chmod tests and elsewhere can be replaced by octal with 0oNNN format.

bovine
bovine requested changes Jun 24, 2020
View reviewed changes
tests/chmod.test Outdated Show resolved Hide resolved
tests/fstat.test Outdated Show resolved Hide resolved
tests/fstat.test Outdated Show resolved Hide resolved
ramikhaldi and others added 3 commits September 30, 2021 23:38
@ramikhaldi @bovine
Update tests/fstat.test
b99a668 
Co-authored-by: Jeff Lawson <[email protected]>
@ramikhaldi @bovine
Update tests/fstat.test
342c09e 
Co-authored-by: Jeff Lawson <[email protected]>
@ramikhaldi @bovine
Update tests/chmod.test
e105385 
Co-authored-by: Jeff Lawson <[email protected]>
@ramikhaldi
Copy link
Author

Who can merge it ?

@bovine
Copy link
Member

bovine commented Nov 5, 2021

I've asked @resuna to see if he has any further feedback on his earlier comments.

@resuna
Copy link
Member

resuna commented Nov 5, 2021

Can you respond to my comments, above?

resuna
resuna requested changes Nov 5, 2021
View reviewed changes
configure Show resolved Hide resolved
generic/tclExtend.h Show resolved Hide resolved
@ramikhaldi
Copy link
Author

Can you respond to my comments, above?

If you have some change requests, then you can submit them then we merge and run it into the CI. BTW, the tests were green with tcl8.6 on my local machine last year..

@ramikhaldi ramikhaldi requested a review from resuna November 5, 2021 23:19
resuna
resuna requested changes Nov 5, 2021
View reviewed changes
generic/tclExtend.h
Comment on lines +55 to +58
#define CONST const
#define VOID void
#define panic Tcl_Panic

Copy link
Member

@resuna resuna Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to build without these lines. They were not previously needed. If there are other changes necessary to make it work they need to be made elsewhere.

@ramikhaldi
Copy link
Author

As I am planning to fork this project again for fixing the windows makefile, I have to close the pull request temporarily. I don't know, If it will work fine, but let's try.

@ramikhaldi ramikhaldi closed this Nov 19, 2021
@mwagner-webdev
Copy link

@ramikhaldi @resuna Is this compatiblity project still underway? I made some similar fixes (visible in my fork https://github.com/mwagner-webdev/tclx) before coming across this PR. I could incorporate your changes (especially the test fixes) if you want, and make a new PR

@ramikhaldi
Copy link
Author

As far as I can remember, this was ready to be merged, but nothing has happened yet. I had to close it because I needed to fork the project for another pull request. Feel free to incorporate my changes into your own PR, or alternatively, this PR can be reopened and merged. I’m open to both approaches...

@mwagner-webdev
Copy link

Feel free to incorporate my changes into your own PR, or alternatively, this PR can be reopened and merged. I’m open to both approaches...

You can reopen it if Github allows that... Did you get all tests to pass already? If the tests pass I think the only task left should be removing the #defines (comment above) which is just a search/replace.

So if it's easy for you to reopen the PR, it's probably faster for you to do it.

I had to close it because I needed to fork the project for another pull request.

Jus a heads up, Github does support creating a PR to master/main in the original repo from a non-main branch in a fork, so you might have been able to use two different branches for this instead of a whole new fork

@resuna
Copy link
Member

resuna commented Sep 1, 2023 •
edited
Loading

Tcl9 is still in flux, so we've been hanging fire on the Tclx Tcl9 work.

@bovine do you feel comfortable merging this?

@mwagner-webdev
Copy link

mwagner-webdev commented Sep 2, 2023 •
edited
Loading

I now had the time to check out the code and play around with it a little bit.

Some remaining issues I noticed on my machine, trying to build this with the latest Tcl trunk:

  • The configure script still issues a warning about the core being a non-threaded build even though it is:
    ... checking for pthread_mutex_init in -lpthread... yes checking for building with threads... yes (default) configure: WARNING: --enable-threads requested, but building against a Tcl that is NOT thread-enabled. This is an OK configuration that will also run in a thread-enabled core. ...
    This is probably related to a bunch of changes that were done in 2018 on the use of TCL_THREADS (this commit and following). Unfortunately I am not an autotools pro and have no idea how to fix it, let alone what the best practice for fixing it would be.
  • Profiler tests are failing (output here)
  • A single test failure in fstat.test (caused by an errant line break introduced by this PR)

@mwagner-webdev
Copy link

also Tcl_Size warnings when building a version without compatibility macros (see https://core.tcl-lang.org/tips/doc/trunk/tip/661.md). I can take a look at those

@chrstphrchvz chrstphrchvz mentioned this pull request Nov 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@resuna resuna resuna requested changes

@bovine bovine bovine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ramikhaldi @resuna @bovine @mwagner-webdev @rkhaldi